feat: add codex context budget controls#62
Conversation
|
@coderabbitai re-review |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds two Codex context-budget settings ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WebUI as Web UI (app.js)
participant API as Backend (cli.js / MCP)
participant FS as Config Store
User->>WebUI: open settings / edit numeric fields
WebUI->>API: GET /api/status
API->>FS: read config (include model_* keys)
FS-->>API: config contents
API-->>WebUI: status payload (modelContextWindow, modelAutoCompactTokenLimit)
User->>WebUI: applyCodexConfigDirect(new values)
WebUI->>API: POST codexmate.config.template.get {modelContextWindow, modelAutoCompactTokenLimit, reasoningEffort}
API->>API: validate/normalize inputs, inject/replace TOML keys (preserve BOM/line endings)
API->>FS: writeConfig(new TOML)
FS-->>API: write result
API-->>WebUI: apply result
WebUI->>WebUI: loadAll() (refresh status)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Stylelint (17.5.0)web-ui/styles.cssJSONError: JSON Error in /package.json: 🔧 ast-grep (0.42.0)web-ui/app.jsComment |
|
Please limit all review feedback to correctness, safety, and quality only within the existing design. Do not suggest breaking changes, refactors, or alterations to existing APIs, interfaces, or behavior. 🧠 Learnings used✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
web-ui/styles.css (1)
1508-1556:⚠️ Potential issue | 🟠 MajorFixed 32px header action buttons override mobile 44px tap targets.
The new high-specificity rule fixes
.btn-tool/.btn-tool-compactto32px, which can bypass mobile touch-target sizing and create an accessibility regression on small screens.🔧 Non-breaking CSS fix
.selector-header .trash-header-actions > .btn-tool, .selector-header .trash-header-actions > .btn-tool-compact { @@ height: 32px; min-height: 32px; @@ } + +@media (max-width: 540px) { + .selector-header .trash-header-actions > .btn-tool, + .selector-header .trash-header-actions > .btn-tool-compact { + height: 44px; + min-height: 44px; + } +}Based on learnings, this keeps feedback strictly to correctness/accessibility quality without introducing breaking API or behavior changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/styles.css` around lines 1508 - 1556, The rule forcing .selector-header .trash-header-actions > .btn-tool and .btn-tool-compact to 32px creates an accessibility regression on touch devices; change that selector to avoid hard-coded 32px by replacing height:32px; min-height:32px with a responsive rule that preserves desktop layout but ensures mobile tap targets are >=44px — e.g. remove the fixed height and keep min-width:96px, or set min-height:44px and height:auto, or add an overriding media query like `@media` (max-width:480px){ .selector-header .trash-header-actions > .btn-tool, .selector-header .trash-header-actions > .btn-tool-compact { min-height:44px; height:auto; } } so small screens get 44px tap targets while larger screens keep the intended compact sizing.cli.js (2)
3342-3408:⚠️ Potential issue | 🟠 MajorValidate these fields when applying an edited template, too.
The new positive-integer guard only runs on the generation/read side. A hand-edited template can still persist invalid
model_context_window/model_auto_compact_token_limitvalues throughapplyConfigTemplate(), and the read helper will later collapse that bad state to"", which makes the write hard to diagnose. Reject invalid values beforewriteConfig(...).💡 Minimal fix
function applyConfigTemplate(params = {}) { const template = typeof params.template === 'string' ? params.template : ''; if (!template.trim()) { return { error: '模板内容不能为空' }; } let parsed; try { parsed = toml.parse(template); } catch (e) { return { error: `模板 TOML 解析失败: ${e.message}` }; } + + if ( + Object.prototype.hasOwnProperty.call(parsed, 'model_context_window') + && normalizePositiveIntegerParam(parsed.model_context_window) === null + ) { + return { error: '模板中的 model_context_window 必须是正整数' }; + } + if ( + Object.prototype.hasOwnProperty.call(parsed, 'model_auto_compact_token_limit') + && normalizePositiveIntegerParam(parsed.model_auto_compact_token_limit) === null + ) { + return { error: '模板中的 model_auto_compact_token_limit 必须是正整数' }; + } if (!parsed.model_provider || typeof parsed.model_provider !== 'string') { return { error: '模板缺少 model_provider' }; }Also applies to: 3414-3421
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 3342 - 3408, The template-apply path doesn't validate positive-integer fields, so invalid values in an edited template can be persisted; update applyConfigTemplate to validate the extracted model_auto_compact_token_limit and model_context_window using normalizePositiveIntegerParam (or reuse applyPositiveIntegerConfigToTemplate's logic) before calling writeConfig: if a value is present but normalizePositiveIntegerParam(value) returns null, reject the template (throw/return error) or remove/replace the invalid field with a safe default, ensuring writeConfig never persists invalid non-positive/inexact integer values. Ensure you reference the same keys 'model_auto_compact_token_limit' and 'model_context_window' and keep behavior consistent with getConfigTemplate's normalization.
10042-10060:⚠️ Potential issue | 🟡 MinorWrap the
statuscase in a block to prevent switch-scope variable conflicts.The case statement declares lexical variables (
const statusConfigResult,const config, etc.) without a block scope, which Biome'snoSwitchDeclarationsrule flags as an error in biome.json. This pattern creates potential TDZ (temporal dead zone) and redeclaration issues if other cases are added or modified later.Fix
- case 'status': + case 'status': { const statusConfigResult = readConfigOrVirtualDefault(); const config = statusConfigResult.config; const serviceTier = typeof config.service_tier === 'string' ? config.service_tier.trim() : ''; const modelReasoningEffort = typeof config.model_reasoning_effort === 'string' ? config.model_reasoning_effort.trim() : ''; const modelContextWindow = readPositiveIntegerConfigValue(config, 'model_context_window'); const modelAutoCompactTokenLimit = readPositiveIntegerConfigValue(config, 'model_auto_compact_token_limit'); result = { provider: config.model_provider || '未设置', model: config.model || '未设置', serviceTier, modelReasoningEffort, modelContextWindow, modelAutoCompactTokenLimit, configReady: !statusConfigResult.isVirtual, configErrorType: statusConfigResult.errorType || '', configNotice: statusConfigResult.reason || '', initNotice: consumeInitNotice() }; break; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 10042 - 10060, Wrap the entire case 'status' body in its own block (e.g., { ... }) so the lexical declarations (statusConfigResult, config, serviceTier, modelReasoningEffort, modelContextWindow, modelAutoCompactTokenLimit, and result) are block-scoped; this prevents switch-scope TDZ/redeclaration issues flagged by Biome's noSwitchDeclarations rule—locate the case 'status' branch in cli.js and enclose the existing statements between an opening and closing brace immediately after the case label.
🧹 Nitpick comments (1)
tests/unit/config-tabs-ui.test.mjs (1)
100-102: Avoid pinning the full inlinestyleblob here.These regexes depend on exact attribute order and serialization, so harmless template cleanup can fail the test even when the trash actions still render correctly. The stable contract here is the class/click wiring; the exact sizing rules are already covered by the
styles.cssassertions below.💡 Less brittle assertion pattern
- assert.match(html, /class="settings-tab-actions trash-header-actions" style="display:flex; flex-wrap:nowrap; align-items:stretch; justify-content:flex-end; gap:8px; margin-left:auto;"/); - assert.match(html, /<button class="btn-tool btn-tool-compact" style="margin:0; width:96px; min-width:96px; height:32px; min-height:32px; padding:0 10px; display:flex; align-items:center; justify-content:center; align-self:stretch; line-height:1; vertical-align:top; position:relative; top:0; transform:none;" `@click`="loadSessionTrash\(\{ forceRefresh: true \}\)"/); - assert.match(html, /<button class="btn-tool btn-tool-compact" style="margin:0; width:96px; min-width:96px; height:32px; min-height:32px; padding:0 10px; display:flex; align-items:center; justify-content:center; align-self:stretch; line-height:1; vertical-align:top; position:relative; top:0; transform:none;" `@click`="clearSessionTrash"/); + assert.match(html, /class="settings-tab-actions trash-header-actions"/); + assert.match(html, /<button class="btn-tool btn-tool-compact"[\s\S]*@click="loadSessionTrash\(\{ forceRefresh: true \}\)"/); + assert.match(html, /<button class="btn-tool btn-tool-compact"[\s\S]*@click="clearSessionTrash"/);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/config-tabs-ui.test.mjs` around lines 100 - 102, The assertions are brittle because they match the full inline style blob; instead keep the stable contract by asserting the presence of the CSS classes and the click handlers only. Update the assertions in tests/unit/config-tabs-ui.test.mjs to stop matching the full style string and instead assert that the container has class names "settings-tab-actions" and "trash-header-actions", that the buttons include "btn-tool btn-tool-compact", and that the buttons have `@click` handlers referencing loadSessionTrash (with its forceRefresh call) and clearSessionTrash (use simple regexes like /@click="loadSessionTrash/ and /@click="clearSessionTrash/ or a relaxed match for the forceRefresh object) so the test no longer depends on exact attribute serialization.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web-ui/app.js`:
- Around line 3245-3286: The blur/reset handlers (onModelContextWindowBlur,
onModelAutoCompactTokenLimitBlur, resetCodexContextBudgetDefaults) drop a second
change if applyCodexConfigDirect is already in flight; instead add a small
queuing mechanism: keep a pendingCodexConfig object and a boolean (e.g.,
applyingCodexConfig) alongside applyCodexConfigDirect, so when a handler wants
to apply but applyingCodexConfig is true it merges its new fields into
pendingCodexConfig and returns; when an apply finishes, if pendingCodexConfig is
non-empty call applyCodexConfigDirect again with that merged payload (clearing
pending after scheduling) so the latest change is applied and loadAll won't
overwrite with stale server values; update applyCodexConfigDirect callers and
ensure resetCodexContextBudgetDefaults also sets pendingCodexConfig when an
apply is active.
---
Outside diff comments:
In `@cli.js`:
- Around line 3342-3408: The template-apply path doesn't validate
positive-integer fields, so invalid values in an edited template can be
persisted; update applyConfigTemplate to validate the extracted
model_auto_compact_token_limit and model_context_window using
normalizePositiveIntegerParam (or reuse applyPositiveIntegerConfigToTemplate's
logic) before calling writeConfig: if a value is present but
normalizePositiveIntegerParam(value) returns null, reject the template
(throw/return error) or remove/replace the invalid field with a safe default,
ensuring writeConfig never persists invalid non-positive/inexact integer values.
Ensure you reference the same keys 'model_auto_compact_token_limit' and
'model_context_window' and keep behavior consistent with getConfigTemplate's
normalization.
- Around line 10042-10060: Wrap the entire case 'status' body in its own block
(e.g., { ... }) so the lexical declarations (statusConfigResult, config,
serviceTier, modelReasoningEffort, modelContextWindow,
modelAutoCompactTokenLimit, and result) are block-scoped; this prevents
switch-scope TDZ/redeclaration issues flagged by Biome's noSwitchDeclarations
rule—locate the case 'status' branch in cli.js and enclose the existing
statements between an opening and closing brace immediately after the case
label.
In `@web-ui/styles.css`:
- Around line 1508-1556: The rule forcing .selector-header .trash-header-actions
> .btn-tool and .btn-tool-compact to 32px creates an accessibility regression on
touch devices; change that selector to avoid hard-coded 32px by replacing
height:32px; min-height:32px with a responsive rule that preserves desktop
layout but ensures mobile tap targets are >=44px — e.g. remove the fixed height
and keep min-width:96px, or set min-height:44px and height:auto, or add an
overriding media query like `@media` (max-width:480px){ .selector-header
.trash-header-actions > .btn-tool, .selector-header .trash-header-actions >
.btn-tool-compact { min-height:44px; height:auto; } } so small screens get 44px
tap targets while larger screens keep the intended compact sizing.
---
Nitpick comments:
In `@tests/unit/config-tabs-ui.test.mjs`:
- Around line 100-102: The assertions are brittle because they match the full
inline style blob; instead keep the stable contract by asserting the presence of
the CSS classes and the click handlers only. Update the assertions in
tests/unit/config-tabs-ui.test.mjs to stop matching the full style string and
instead assert that the container has class names "settings-tab-actions" and
"trash-header-actions", that the buttons include "btn-tool btn-tool-compact",
and that the buttons have `@click` handlers referencing loadSessionTrash (with its
forceRefresh call) and clearSessionTrash (use simple regexes like
/@click="loadSessionTrash/ and /@click="clearSessionTrash/ or a relaxed match
for the forceRefresh object) so the test no longer depends on exact attribute
serialization.
🪄 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: 3f1b517e-35b8-473b-8673-1e10da77e878
📒 Files selected for processing (6)
cli.jstests/e2e/test-config.jstests/unit/config-tabs-ui.test.mjsweb-ui/app.jsweb-ui/index.htmlweb-ui/styles.css
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-31T12:37:50.988Z
Learning: In PR `#59` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-30T04:47:15.339Z
Learning: In PR `#55` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
🪛 Biome (2.4.9)
cli.js
[error] 10047-10047: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
(lint/correctness/noSwitchDeclarations)
[error] 10048-10048: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (11)
web-ui/index.html (2)
471-517: 新增上下文预算输入区实现完整且交互闭环清晰。Line 471 到 Line 517 的输入绑定、数字清洗、失焦/回车提交与“重置默认值”禁用条件配合合理，和现有模板写入流程一致。
1171-1175: 回收站头部操作区布局调整有效。Line 1171 到 Line 1175 保留了刷新/清空动作与禁用保护，并实现了右侧对齐和统一按钮尺寸，行为上保持稳定。
tests/e2e/test-config.js (4)
25-28: Status API 对新字段的契约断言补充到位。Line 25 到 Line 28 同时覆盖字段存在性与默认值正确性，能及时发现状态负载回归。
90-100: Context Budget 独立模板用例覆盖有效。Line 90 到 Line 100 新增单独场景验证
model_context_window与model_auto_compact_token_limit写入，测试意图清晰。
106-108: Combined 场景已正确纳入上下文预算断言。Line 106 到 Line 108、Line 113 到 Line 114 将新字段纳入组合用例，能更好覆盖多参数并存时的模板输出。
Also applies to: 113-114
120-123: Combined xhigh 场景补全一致且必要。Line 120 到 Line 123、Line 127 到 Line 128 对 xhigh 组合路径增加上下文预算校验，覆盖面完整。
Also applies to: 127-128
web-ui/styles.css (2)
1709-1719: Codex config grid/field styles look good.
auto-fitwith a 240px minimum andmin-width: 0on fields is a solid, resilient layout for the new context budget inputs.
2145-2158: Trash action button grid alignment is clean and consistent.The 2-column action grid plus full-width
.btn-minicentering improves visual consistency and click predictability.web-ui/app.js (1)
3226-3243: Nice to see the parsing rules centralized.Reusing one normalizer for the new numeric fields keeps the blur, template, and direct-apply flows aligned.
cli.js (2)
96-97: Good default wiring.The new context-budget defaults are centralized once and reused in both the fallback template and the bootstrap config, which keeps first-run and template-generation behavior aligned.
Also applies to: 231-249, 3175-3188
11529-11548: Nice MCP/workflow consistency.The same fields are now exposed through status, workflow input, and the MCP tool schema, which should keep non-Web callers aligned with the Web UI flow.
Also applies to: 11628-11675, 12225-12242
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 (1)
cli.js (1)
10042-10060:⚠️ Potential issue | 🟠 MajorWrap the
statusswitch branch in braces.The case 'status' branch declares multiple
constbindings directly under the case label without block scoping, which Biome'snoSwitchDeclarationsrule (configured as an error in biome.json) flags as a violation. Wrapping the branch body in braces will create a separate lexical scope and resolve the linting error.🔧 Minimal fix
- case 'status': + case 'status': { const statusConfigResult = readConfigOrVirtualDefault(); const config = statusConfigResult.config; const serviceTier = typeof config.service_tier === 'string' ? config.service_tier.trim() : ''; const modelReasoningEffort = typeof config.model_reasoning_effort === 'string' ? config.model_reasoning_effort.trim() : ''; const modelContextWindow = readPositiveIntegerConfigValue(config, 'model_context_window'); const modelAutoCompactTokenLimit = readPositiveIntegerConfigValue(config, 'model_auto_compact_token_limit'); result = { provider: config.model_provider || '未设置', model: config.model || '未设置', serviceTier, modelReasoningEffort, modelContextWindow, modelAutoCompactTokenLimit, configReady: !statusConfigResult.isVirtual, configErrorType: statusConfigResult.errorType || '', configNotice: statusConfigResult.reason || '', initNotice: consumeInitNotice() }; break; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 10042 - 10060, The case 'status' switch branch currently declares multiple const bindings (e.g., statusConfigResult from readConfigOrVirtualDefault(), serviceTier, modelReasoningEffort, modelContextWindow via readPositiveIntegerConfigValue, modelAutoCompactTokenLimit, and uses consumeInitNotice()) directly under the case label; wrap the entire branch body in a block (add { ... } after case 'status': and close it before the following break/next case) so these declarations are lexically scoped and satisfy the noSwitchDeclarations rule, preserving the existing variable names and return/object assignment to result.
♻️ Duplicate comments (1)
web-ui/app.js (1)
3404-3469:⚠️ Potential issue | 🟠 MajorDon't drop the latest budget edit while a Codex apply is already running.
Line 3405 still returns immediately when
codexApplyingis true. With the new blur/reset handlers at Lines 3245-3285, a user can blur one budget field, then change the other before the first request finishes. That second edit is discarded, andloadAll()then restores the stale server value back into the input.💡 Non-breaking fix
async applyCodexConfigDirect(options = {}) { - if (this.codexApplying) return; + if (this.codexApplying) { + this._pendingCodexApplyOptions = { + ...(this._pendingCodexApplyOptions || {}), + ...options + }; + return; + } @@ } catch (e) { this.showMessage('应用失败', 'error'); } finally { this.codexApplying = false; + const pendingOptions = this._pendingCodexApplyOptions; + this._pendingCodexApplyOptions = null; + if (pendingOptions) { + await this.applyCodexConfigDirect(pendingOptions); + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/app.js` around lines 3404 - 3469, The handler applyCodexConfigDirect currently bails out immediately if this.codexApplying is true, which drops concurrent edits; instead implement a lightweight pending-apply mechanism: when codexApplying is true, capture the latest options (or the current inputs like this.modelContextWindowInput, this.modelAutoCompactTokenLimitInput, provider/model) into a this._pendingCodexOptions object and return, and at the end of the current run (in the finally block where codexApplying is set to false) check if this._pendingCodexOptions exists — if so, clear the flag, copy it to a local var, and call applyCodexConfigDirect(localPendingOptions) (preserving options.silent semantics); use the existing symbols applyCodexConfigDirect, codexApplying, and loadAll so the second edit is applied immediately after the first finishes rather than being discarded.
🧹 Nitpick comments (1)
web-ui/index.html (1)
1171-1178: Consider extracting repetitive inline styles to a CSS class.Both buttons share identical inline style declarations (~100 characters each). This works but creates maintenance overhead. Consider adding a utility class in
styles.cssfor this pattern.♻️ Optional: Extract to CSS class
In
web-ui/styles.css:.trash-header-btn { margin: 0; width: 96px; min-width: 96px; height: 32px; min-height: 32px; padding: 0 10px; display: flex; align-items: center; justify-content: center; align-self: stretch; line-height: 1; }Then simplify the HTML:
-<button class="btn-tool btn-tool-compact" style="margin:0; width:96px; min-width:96px; height:32px; min-height:32px; padding:0 10px; display:flex; align-items:center; justify-content:center; align-self:stretch; line-height:1; vertical-align:top; position:relative; top:0; transform:none;" `@click`="loadSessionTrash({ forceRefresh: true })" :disabled="sessionTrashLoading || sessionTrashClearing"> +<button class="btn-tool btn-tool-compact trash-header-btn" `@click`="loadSessionTrash({ forceRefresh: true })" :disabled="sessionTrashLoading || sessionTrashClearing">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/index.html` around lines 1171 - 1178, The two buttons with classes "btn-tool btn-tool-compact" repeat an identical inline style block; extract that style into a reusable CSS class (e.g., .trash-header-btn) in styles.css and replace the inline style on both buttons with that class, keeping existing attributes and Vue bindings (`@click`, :disabled, text interpolation) intact so only styling is moved out of the HTML and not behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli.js`:
- Around line 3395-3408: The template update only adds keys if the param is set
or already present, so upgraded configs never get the new
model_context_window/model_auto_compact_token_limit defaults; modify the logic
so that applyPositiveIntegerConfigToTemplate ensures the key is present with the
new default when absent: either call applyPositiveIntegerConfigToTemplate with
the default value (not undefined) for 'model_context_window' and
'model_auto_compact_token_limit' when params.modelContextWindow /
params.modelAutoCompactTokenLimit are undefined, or change
applyPositiveIntegerConfigToTemplate to insert the default into template if the
key is missing (use the canonical default constants), referencing the symbols
template, applyPositiveIntegerConfigToTemplate, params.modelContextWindow, and
params.modelAutoCompactTokenLimit and the keys 'model_context_window' /
'model_auto_compact_token_limit'.
In `@web-ui/styles.css`:
- Around line 1534-1556: The rules targeting .selector-header
.trash-header-actions > .btn-tool and .btn-tool-compact set a fixed
height/min-height: 32px which shrinks mobile touch targets; to fix, remove the
fixed height declaration and change the min-height to 44px (or replace
min-height: 32px with min-height: 44px) so buttons retain adequate tap size on
mobile while allowing larger/heavier content; update the selectors in the shown
block (remove height: 32px and ensure min-height: 44px) so specificity stays the
same but touch target sizing is restored.
---
Outside diff comments:
In `@cli.js`:
- Around line 10042-10060: The case 'status' switch branch currently declares
multiple const bindings (e.g., statusConfigResult from
readConfigOrVirtualDefault(), serviceTier, modelReasoningEffort,
modelContextWindow via readPositiveIntegerConfigValue,
modelAutoCompactTokenLimit, and uses consumeInitNotice()) directly under the
case label; wrap the entire branch body in a block (add { ... } after case
'status': and close it before the following break/next case) so these
declarations are lexically scoped and satisfy the noSwitchDeclarations rule,
preserving the existing variable names and return/object assignment to result.
---
Duplicate comments:
In `@web-ui/app.js`:
- Around line 3404-3469: The handler applyCodexConfigDirect currently bails out
immediately if this.codexApplying is true, which drops concurrent edits; instead
implement a lightweight pending-apply mechanism: when codexApplying is true,
capture the latest options (or the current inputs like
this.modelContextWindowInput, this.modelAutoCompactTokenLimitInput,
provider/model) into a this._pendingCodexOptions object and return, and at the
end of the current run (in the finally block where codexApplying is set to
false) check if this._pendingCodexOptions exists — if so, clear the flag, copy
it to a local var, and call applyCodexConfigDirect(localPendingOptions)
(preserving options.silent semantics); use the existing symbols
applyCodexConfigDirect, codexApplying, and loadAll so the second edit is applied
immediately after the first finishes rather than being discarded.
---
Nitpick comments:
In `@web-ui/index.html`:
- Around line 1171-1178: The two buttons with classes "btn-tool
btn-tool-compact" repeat an identical inline style block; extract that style
into a reusable CSS class (e.g., .trash-header-btn) in styles.css and replace
the inline style on both buttons with that class, keeping existing attributes
and Vue bindings (`@click`, :disabled, text interpolation) intact so only styling
is moved out of the HTML and not behavior.
🪄 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: 56a969bc-248a-4ced-bd37-d9a7762d0111
📒 Files selected for processing (6)
cli.jstests/e2e/test-config.jstests/unit/config-tabs-ui.test.mjsweb-ui/app.jsweb-ui/index.htmlweb-ui/styles.css
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-31T12:37:50.988Z
Learning: In PR `#59` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-30T04:47:15.339Z
Learning: In PR `#55` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: SurviveM
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-29T02:42:32.761Z
Learning: The repository maintainer SurviveM (GitHub: SurviveM, repo: SakuraByteCore/codexmate) prefers that CodeRabbit review suggestions do not introduce breaking changes. Reviews should focus on correctness, safety, and quality without recommending refactors or changes that would break existing APIs, interfaces, or behavior.
🪛 Biome (2.4.9)
cli.js
[error] 10047-10047: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
(lint/correctness/noSwitchDeclarations)
[error] 10048-10048: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (9)
web-ui/index.html (1)
471-516: New context budget controls are well-implemented.The inputs correctly use
inputmode="numeric"for mobile UX, have proper label associations viaid/for, and follow the existing pattern for blur/Enter commit handlers. The reset button disabled state properly guards against concurrent operations.One minor observation: the
@inputsanitization and@blur/@keydown.enterhandlers look correct, but ensuresanitizePositiveIntegerDraft,onModelContextWindowBlur, andonModelAutoCompactTokenLimitBlurare implemented inapp.jsto avoid runtime errors.tests/e2e/test-config.js (4)
25-28: Status API assertions correctly validate new context budget fields.The assertions check both field presence and exact default values (190000, 185000), which aligns with the UI defaults and provides strong regression coverage.
90-99: Context Budget template test provides good isolated coverage.The test uses distinct values (200000, 195000) from the defaults, ensuring the template generation actually incorporates the provided values rather than falling back to defaults. The regex patterns correctly match TOML integer format without quotes.
106-114: Combined test properly extended with context budget assertions.The test now validates all configuration fields together, ensuring they coexist correctly in the generated template.
120-128: Combined xhigh test uses different budget values, improving coverage.Using 210000/200000 instead of the defaults (190000/185000) ensures this test exercises the template generation path independently from the "Combined" test above.
web-ui/styles.css (4)
1508-1509: Alignment update looks correct.Right-aligning and centering the settings header actions is consistent with the updated action-bar layout.
1524-1532: Trash header action container is well-structured.The dedicated grid container cleanly enforces consistent button slots and right alignment.
1709-1719: Codex config layout utilities are solid.The responsive grid plus
min-width: 0field guard is a good addition for stable two-field rendering.
2145-2158: Trash item action grid update looks good.The two-column grid with full-width mini buttons gives consistent action geometry and improves visual stability.
|
@coderabbitai re-review |
|
Please limit all review feedback to correctness, safety, and quality only within the existing design. Do not suggest breaking changes, refactors, or alterations to existing APIs, interfaces, or behavior. 🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cli.js (1)
3395-3408:⚠️ Potential issue | 🟠 MajorStill unresolved: legacy configs never pick up the new context-budget defaults.
These paths only surface
model_context_window/model_auto_compact_token_limitwhen the existing file already has them or the caller passes explicit values. An upgradedconfig.tomlthat predates these keys therefore still generates templates without the new lines, and status keeps returning''instead of the intended defaults, so the new UI/MCP flow behaves like the feature is unset on legacy installs. Please seed the default values when the keys are absent before applying overrides, and surface the same defaults in status for loaded configs that simply omit them.Also applies to: 3414-3421
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 3395 - 3408, The template-building logic currently only injects 'model_context_window' and 'model_auto_compact_token_limit' when the config already contains them or the caller passes explicit params; seed the template with the new default values before applying overrides so legacy config.toml files get the new lines. Update the code path that constructs `template` (the blocks surrounding calls to applyPositiveIntegerConfigToTemplate) to first ensure default entries for 'model_context_window' and 'model_auto_compact_token_limit' are present using the intended defaults, then call applyPositiveIntegerConfigToTemplate with `params.modelContextWindow` and `params.modelAutoCompactTokenLimit`; also mirror this default-seeding in the status/load logic that reads configs so status returns the default string instead of ''. Ensure you modify both occurrences noted (the blocks invoking applyPositiveIntegerConfigToTemplate around `params.modelContextWindow` and `params.modelAutoCompactTokenLimit`) so legacy installs surface the defaults.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cli.js`:
- Around line 3395-3408: The template-building logic currently only injects
'model_context_window' and 'model_auto_compact_token_limit' when the config
already contains them or the caller passes explicit params; seed the template
with the new default values before applying overrides so legacy config.toml
files get the new lines. Update the code path that constructs `template` (the
blocks surrounding calls to applyPositiveIntegerConfigToTemplate) to first
ensure default entries for 'model_context_window' and
'model_auto_compact_token_limit' are present using the intended defaults, then
call applyPositiveIntegerConfigToTemplate with `params.modelContextWindow` and
`params.modelAutoCompactTokenLimit`; also mirror this default-seeding in the
status/load logic that reads configs so status returns the default string
instead of ''. Ensure you modify both occurrences noted (the blocks invoking
applyPositiveIntegerConfigToTemplate around `params.modelContextWindow` and
`params.modelAutoCompactTokenLimit`) so legacy installs surface the defaults.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e74a6669-33ff-4019-904c-7b2139bdbba3
📒 Files selected for processing (7)
cli.jstests/e2e/test-config.jstests/unit/config-tabs-ui.test.mjstests/unit/provider-share-command.test.mjsweb-ui/app.jsweb-ui/index.htmlweb-ui/styles.css
🚧 Files skipped from review as they are similar to previous changes (1)
- web-ui/app.js
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-31T12:37:50.988Z
Learning: In PR `#59` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-30T04:47:15.339Z
Learning: In PR `#55` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: SurviveM
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-29T02:42:32.761Z
Learning: The repository maintainer SurviveM (GitHub: SurviveM, repo: SakuraByteCore/codexmate) prefers that CodeRabbit review suggestions do not introduce breaking changes. Reviews should focus on correctness, safety, and quality without recommending refactors or changes that would break existing APIs, interfaces, or behavior.
🔇 Additional comments (15)
web-ui/styles.css (3)
1524-1556: Trash header action sizing and mobile override look correct.The new grid action container and the mobile 44px override are consistent and address compact-vs-touch behavior cleanly.
Also applies to: 4435-4439
1709-1719: Codex context-budget form grid styles are solid.The grid and field constraints (
min-width: 0, responsiveminmax) are appropriate and should prevent overflow issues in narrow layouts.
2145-2158: Trash item action button alignment update looks good.Using a two-column grid with full-width mini buttons improves consistency and preserves predictable click targets.
web-ui/index.html (2)
471-517: Context-budget controls are correctly wired.The new inputs, sanitization hooks, blur/enter commit handlers, and reset button wiring are consistent with the expected Codex apply flow.
1171-1178: Trash header action structure update is clean and consistent.The consolidated
trash-header-actionscontainer matches the CSS/test updates and keeps the actions straightforward.tests/unit/config-tabs-ui.test.mjs (2)
32-42: Good coverage added for Codex context-budget wiring and settings-trash structure.These assertions directly validate the new bindings, handlers, and header action markup, which reduces UI regression risk.
Also applies to: 100-103, 201-209
291-315: CSS regression assertions are well targeted.The new checks around trash action sizing/layout and mobile overrides are useful and match the behavior introduced in
web-ui/styles.css.Also applies to: 320-343
tests/e2e/test-config.js (5)
25-28: Solid status payload coverage for new context-budget fields.These assertions correctly lock in both field presence and expected default values in the status response.
90-99: Good template rendering check for explicit context-budget inputs.This verifies both
model_context_windowandmodel_auto_compact_token_limitare emitted into generated TOML as intended.
106-114: Combined-template assertions are well-scoped.Nice coverage ensuring service tier, reasoning effort, and both context-budget keys coexist correctly in one template flow.
120-128: Great xhigh variant coverage with context-budget values.This complements the combined test and helps guard alternate reasoning-effort paths.
130-159: Validation-path tests for invalid context-budget values look strong.These negative cases correctly assert rejection behavior and expected error messaging for non-positive/non-integer inputs.
tests/unit/provider-share-command.test.mjs (3)
149-200: Nice negative-path unit coverage for context-budget validation.Good call asserting both exact errors and that
writeConfigis not invoked on invalid templates.
202-204: Useful guard for thestatusswitch-branch scoping regression.This keeps the lexical-branch expectation explicit and easy to detect if accidentally removed.
206-292: Strong concurrency test for pending apply-option replay.This thoroughly verifies in-flight queue behavior, latest pending-budget propagation, cleanup state, and silent-mode messaging expectations.
|
@coderabbitai re-review |
|
Please limit all review feedback to correctness, safety, and quality only within the existing design. Do not suggest breaking changes, refactors, or alterations to existing APIs, interfaces, or behavior. 🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli.js`:
- Around line 3342-3360: normalizePositiveIntegerParam currently returns null
both when the caller omits the field and when they supply an invalid explicit
value, which hides user errors; change its behavior so that when value is
undefined or null it still returns null (meaning "not supplied"), but when a
non-null/non-undefined value is supplied and fails validation (empty string,
non-digits, non-safe-integer, or <= 0) it throws a descriptive Error (e.g.
"invalid positive integer for <paramName>") so callers fail-fast; update
normalizePositiveIntegerParam and any usage sites mentioned (the other block
around 3409-3422 that handles modelContextWindow / modelAutoCompactTokenLimit)
to catch/propagate this error or validate before applying changes.
- Around line 3362-3374: The function applyPositiveIntegerConfigToTemplate
currently normalizes and prepends a line using hardcoded LF/newline and strips
leading whitespace which can remove or mix CRLF and BOM; update it to detect and
preserve the original file's BOM (\uFEFF) and EOL style (use CRLF if the input
template contains '\r\n', otherwise LF) when template is a string, and use those
preserved values when building the inserted line and when trimming leading
newlines so you don't drop the BOM or convert CRLF to LF. Concretely: in
applyPositiveIntegerConfigToTemplate, capture hasBOM =
template.startsWith('\uFEFF'), determine eol = template.includes('\r\n') ?
'\r\n' : '\n', strip the BOM before regex processing then reapply it to the
returned string, replace hardcoded '\n' with eol when prepending `${key} =
${normalized}${eol}`, and change the leading-trim regex (/^\s*\n*/ or similar)
to only remove matching sequences of the detected eol (preserving BOM and
original line endings); apply the same pattern to the similar code at the other
occurrence noted (lines ~3395-3408).
🪄 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: 488c1642-3b45-434b-b1df-c04d2843cf04
📒 Files selected for processing (3)
cli.jstests/e2e/test-config.jstests/unit/provider-share-command.test.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/test-config.js
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-31T12:37:50.988Z
Learning: In PR `#59` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-30T04:47:15.339Z
Learning: In PR `#55` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: SurviveM
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-29T02:42:32.761Z
Learning: The repository maintainer SurviveM (GitHub: SurviveM, repo: SakuraByteCore/codexmate) prefers that CodeRabbit review suggestions do not introduce breaking changes. Reviews should focus on correctness, safety, and quality without recommending refactors or changes that would break existing APIs, interfaces, or behavior.
🔇 Additional comments (8)
tests/unit/provider-share-command.test.mjs (5)
148-200: LGTM!The test correctly validates that
applyConfigTemplaterejects invalid context budget values (zero and non-numeric strings) and ensureswriteConfigis never called on validation failure. The mock setup and assertions are appropriate.
202-250: LGTM!The test properly validates that
getConfigTemplaterestores default context budget values when they're missing from an existing config file. The regex assertions correctly verify the presence of both default values in the generated template.
252-279: LGTM!The test comprehensively validates the fallback behavior of
readPositiveIntegerConfigValue: defaults are used only when keys are missing, invalid values (like0) result in empty string rather than falling back to defaults, and unknown keys return empty string. This correctly tests the distinction between "missing" and "invalid" values.
281-283: LGTM!Simple but valuable assertion ensuring the
statuscase in the CLI uses block scoping (case 'status': { ... }) to properly scope any lexical declarations and avoid temporal dead zone issues across switch cases.
285-371: LGTM!Excellent comprehensive test for the concurrency behavior of
applyCodexConfigDirect. The test correctly validates:
- When a second call arrives while the first is in-flight, options are queued to
_pendingCodexApplyOptions- The recursive call after the first completes uses the merged/pending options
loadAll()is called twice (once per apply completion)- State flags are properly reset after both applies complete
- With
silent: true, no user messages are emittedThe mock API with controlled Promise resolution is a clean approach to test async queuing behavior.
cli.js (3)
96-97: Shared defaults are wired consistently.Using the same constants in the fallback template and the managed bootstrap content keeps fresh installs and virtual defaults aligned.
Also applies to: 229-233, 3175-3176
3428-3439: Legacy configs now surface the restored defaults end-to-end.Nice fix: both the HTTP and MCP status paths now report the context-budget defaults even when older configs omit those keys.
Also applies to: 10074-10087, 11567-11575
3454-3466: Good apply-time validation.Rejecting invalid
model_context_window/model_auto_compact_token_limitvalues before writing the template is the right guardrail here.
|
@coderabbitai re-review |
|
Please limit all review feedback to correctness, safety, and quality only within the existing design. Do not suggest breaking changes, refactors, or alterations to existing APIs, interfaces, or behavior. 🧠 Learnings used✅ Actions performedFull review triggered. |
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 (1)
web-ui/app.js (1)
3405-3480:⚠️ Potential issue | 🟠 MajorSnapshot state-backed Codex fields before queueing a reapply.
When
codexApplyingis already true, onlyoptionsis preserved. Lines 3204-3214 still call this method without passingcurrentModel,serviceTier, ormodelReasoningEffort, so Line 3471 can restore the old server state vialoadAll()before the queued apply reruns. The user's later model/service-tier/reasoning change is still silently lost.💡 Non-breaking fix
async applyCodexConfigDirect(options = {}) { + const provider = String( + options.provider !== undefined ? options.provider : (this.currentProvider || '') + ).trim(); + const model = String( + options.model !== undefined ? options.model : (this.currentModel || '') + ).trim(); + const serviceTier = options.serviceTier !== undefined + ? options.serviceTier + : this.serviceTier; + const reasoningEffort = options.reasoningEffort !== undefined + ? options.reasoningEffort + : this.modelReasoningEffort; + const queuedModelContextWindow = options.modelContextWindow !== undefined + ? options.modelContextWindow + : this.modelContextWindowInput; + const queuedModelAutoCompactTokenLimit = options.modelAutoCompactTokenLimit !== undefined + ? options.modelAutoCompactTokenLimit + : this.modelAutoCompactTokenLimitInput; + if (this.codexApplying) { this._pendingCodexApplyOptions = { ...(this._pendingCodexApplyOptions || {}), - ...options + provider, + model, + serviceTier, + reasoningEffort, + modelContextWindow: queuedModelContextWindow, + modelAutoCompactTokenLimit: queuedModelAutoCompactTokenLimit, + ...(Object.prototype.hasOwnProperty.call(options, 'silent') + ? { silent: options.silent } + : {}) }; return; } - const provider = (this.currentProvider || '').trim(); - const model = (this.currentModel || '').trim(); if (!provider || !model) { this.showMessage('请选择提供商和模型', 'error'); return; } @@ const tplRes = await api('get-config-template', { provider, model, - serviceTier: this.serviceTier, - reasoningEffort: this.modelReasoningEffort, + serviceTier, + reasoningEffort, modelContextWindow: modelContextWindow.value, modelAutoCompactTokenLimit: modelAutoCompactTokenLimit.value });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/app.js` around lines 3405 - 3480, When queuing a reapply because codexApplying is true, the code only preserves the passed options and loses the UI/server state changes (e.g., currentModel, currentProvider, serviceTier, modelReasoningEffort) made after the original call; update applyCodexConfigDirect to snapshot these state-backed fields into _pendingCodexApplyOptions (merge existing pending options) so that when the queued call is replayed it gets the latest provider/model/serviceTier/reasoning (reference symbols: applyCodexConfigDirect, codexApplying, _pendingCodexApplyOptions, currentModel, currentProvider, serviceTier, modelReasoningEffort); ensure the finally block behavior that reads pendingOptions still awaits applyCodexConfigDirect(pendingOptions) so the queued call uses the merged snapshot.
♻️ Duplicate comments (2)
cli.js (2)
3362-3374:⚠️ Potential issue | 🟡 MinorPreserve BOM and line endings when backfilling legacy defaults.
This helper still prepends LF-only lines, and the template apply path later trims/re-emits the content. With the new auto-backfill at Lines 3395-3408, a CRLF/BOM legacy
config.tomlcan now be reformatted just by round-tripping through the template flow, even if the user never touched these new fields.💡 Keep the template round-trip format-stable
function applyPositiveIntegerConfigToTemplate(template, key, value) { let content = typeof template === 'string' ? template : ''; const normalized = normalizePositiveIntegerParam(value); if (!key || normalized === null) { return content; } + const hadBom = hasUtf8Bom(content); + const lineEnding = detectLineEnding(content); + if (hadBom) { + content = stripUtf8Bom(content); + } const escapedKey = key.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); const pattern = new RegExp(`^\\s*${escapedKey}\\s*=\\s*[^\\n]*\\n?`, 'gmi'); content = content.replace(pattern, ''); - content = content.replace(/^\s*\n*/, ''); - return `${key} = ${normalized}\n${content}`; + content = content.replace(/^\s*\r?\n*/, ''); + const next = `${key} = ${normalized}${lineEnding}${content}`; + return hadBom ? ensureUtf8Bom(next) : next; }- writeConfig(template.trim() + '\n'); + const hadBom = hasUtf8Bom(template); + const lineEnding = detectLineEnding(template); + const body = (hadBom ? stripUtf8Bom(template) : template).trim(); + writeConfig(`${hadBom ? '\uFEFF' : ''}${body}${lineEnding}`);Also applies to: 3395-3408
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 3362 - 3374, The helper applyPositiveIntegerConfigToTemplate (and the auto-backfill path at the block around lines 3395-3408) currently prepends a LF-only line and strips leading content which can drop a BOM or convert CRLF to LF; update applyPositiveIntegerConfigToTemplate to detect and preserve the original template's BOM (UTF-8 BOM) and the template's dominant line ending (CRLF vs LF) and use that same line ending when inserting the new "key = value" line(s), and ensure the function does not remove a leading BOM when calling content.replace(/^\s*\n*/, ''); also apply the same preservation logic in the other auto-backfill code block so round-tripping a legacy config.toml keeps its original BOM and CRLF/LF line endings.
3342-3360:⚠️ Potential issue | 🟠 MajorFail fast on malformed context-budget overrides.
Malformed explicit values like
0,-1, or non-integer text still collapse tonullhere, sogetConfigTemplate()quietly keeps the old/default budget and returns success. That makes the template/MCP workflow path look applied when the override was actually ignored.💡 Minimal non-breaking fix
function getConfigTemplate(params = {}) { + if ( + params.modelContextWindow !== undefined + && params.modelContextWindow !== null + && String(params.modelContextWindow).trim() !== '' + && normalizePositiveIntegerParam(params.modelContextWindow) === null + ) { + return { error: 'modelContextWindow must be a positive integer' }; + } + if ( + params.modelAutoCompactTokenLimit !== undefined + && params.modelAutoCompactTokenLimit !== null + && String(params.modelAutoCompactTokenLimit).trim() !== '' + && normalizePositiveIntegerParam(params.modelAutoCompactTokenLimit) === null + ) { + return { error: 'modelAutoCompactTokenLimit must be a positive integer' }; + } + let content = EMPTY_CONFIG_FALLBACK_TEMPLATE;Also applies to: 3409-3422
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 3342 - 3360, normalizePositiveIntegerParam currently treats malformed explicit inputs (e.g., "0", "-1", non-integer text) as null, silently ignoring overrides; change it to fail fast by throwing a descriptive Error when a non-null/defined input is provided but is not a valid positive integer. Specifically, in normalizePositiveIntegerParam check for presence of an explicit value first, validate that text matches /^\d+$/, parsed is a safe integer and > 0, and if any check fails throw an Error (including the invalid input in the message) instead of returning null; also apply the same change to the duplicate implementation referenced (the other normalize/validation block around the similar logic) and ensure callers like getConfigTemplate handle or propagate the thrown error so malformed overrides are surfaced immediately.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web-ui/styles.css`:
- Around line 1524-1556: The .trash-header-actions block forces two 96px-wide
children so on narrow viewports the buttons overflow; update the layout to allow
shrinking by replacing fixed column sizing and button widths with flexible
rules: change .trash-header-actions grid-auto-columns to a flexible minmax(0,
1fr) (or remove fixed 96px columns) and make the child selectors
(.selector-header .trash-header-actions > .btn-tool and > .btn-tool-compact)
remove fixed width/min-width and instead allow flex/shrink (e.g., use width:
auto; min-width: 0; flex: 1 1 auto or equivalent) so they can compress on small
screens; also ensure the mobile rule around the existing media query (the one
referenced at ~4435) applies the same width-relaxing changes for small
viewports.
- Around line 1709-1719: The grid in .codex-config-grid enforces a 240px minimum
per column which causes overflow on narrow (320px) viewports; update the
grid-template-columns declaration in .codex-config-grid to use a smaller minimum
(e.g., minmax(160px, 1fr)) or a flexible min (e.g., minmax(0, 1fr)) so items can
collapse to a single column on small screens and avoid clipping, and verify
.codex-config-field still has min-width: 0 to allow proper shrinking.
---
Outside diff comments:
In `@web-ui/app.js`:
- Around line 3405-3480: When queuing a reapply because codexApplying is true,
the code only preserves the passed options and loses the UI/server state changes
(e.g., currentModel, currentProvider, serviceTier, modelReasoningEffort) made
after the original call; update applyCodexConfigDirect to snapshot these
state-backed fields into _pendingCodexApplyOptions (merge existing pending
options) so that when the queued call is replayed it gets the latest
provider/model/serviceTier/reasoning (reference symbols: applyCodexConfigDirect,
codexApplying, _pendingCodexApplyOptions, currentModel, currentProvider,
serviceTier, modelReasoningEffort); ensure the finally block behavior that reads
pendingOptions still awaits applyCodexConfigDirect(pendingOptions) so the queued
call uses the merged snapshot.
---
Duplicate comments:
In `@cli.js`:
- Around line 3362-3374: The helper applyPositiveIntegerConfigToTemplate (and
the auto-backfill path at the block around lines 3395-3408) currently prepends a
LF-only line and strips leading content which can drop a BOM or convert CRLF to
LF; update applyPositiveIntegerConfigToTemplate to detect and preserve the
original template's BOM (UTF-8 BOM) and the template's dominant line ending
(CRLF vs LF) and use that same line ending when inserting the new "key = value"
line(s), and ensure the function does not remove a leading BOM when calling
content.replace(/^\s*\n*/, ''); also apply the same preservation logic in the
other auto-backfill code block so round-tripping a legacy config.toml keeps its
original BOM and CRLF/LF line endings.
- Around line 3342-3360: normalizePositiveIntegerParam currently treats
malformed explicit inputs (e.g., "0", "-1", non-integer text) as null, silently
ignoring overrides; change it to fail fast by throwing a descriptive Error when
a non-null/defined input is provided but is not a valid positive integer.
Specifically, in normalizePositiveIntegerParam check for presence of an explicit
value first, validate that text matches /^\d+$/, parsed is a safe integer and >
0, and if any check fails throw an Error (including the invalid input in the
message) instead of returning null; also apply the same change to the duplicate
implementation referenced (the other normalize/validation block around the
similar logic) and ensure callers like getConfigTemplate handle or propagate the
thrown error so malformed overrides are surfaced immediately.
🪄 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: 93f461ec-d7dd-4f6c-b483-faacba1425b1
📒 Files selected for processing (7)
cli.jstests/e2e/test-config.jstests/unit/config-tabs-ui.test.mjstests/unit/provider-share-command.test.mjsweb-ui/app.jsweb-ui/index.htmlweb-ui/styles.css
📜 Review details
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-31T12:37:50.988Z
Learning: In PR `#59` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-30T04:47:15.339Z
Learning: In PR `#55` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: SurviveM
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-29T02:42:32.761Z
Learning: The repository maintainer SurviveM (GitHub: SurviveM, repo: SakuraByteCore/codexmate) prefers that CodeRabbit review suggestions do not introduce breaking changes. Reviews should focus on correctness, safety, and quality without recommending refactors or changes that would break existing APIs, interfaces, or behavior.
📚 Learning: 2026-03-23T15:38:41.305Z
Learnt from: ymkiux
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-23T15:38:41.305Z
Learning: In the SakuraByteCore/codexmate repository, BOM (U+FEFF) characters in Chinese markdown and JavaScript config files (e.g., site/index.md, site/.vitepress/config.js) are intentional and follow the project's encoding convention. Do not flag BOM presence in these files as an issue. The PostCSS BOM parse issue was addressed separately by adding BOM-free site/package.json and site/postcss.config.cjs files.
Applied to files:
cli.js
🔇 Additional comments (14)
tests/e2e/test-config.js (4)
25-28: LGTM! Status API assertions for context budget fields.The assertions correctly verify both presence and exact default values for the new
modelContextWindow(190000) andmodelAutoCompactTokenLimit(185000) fields, aligning with the CLI/backend defaults.
90-99: LGTM! Context budget template generation tests.The test properly verifies that
get-config-templategenerates correct TOML lines for the new budget fields with explicit values (200000, 195000).
130-159: LGTM! Negative validation tests for context budget fields.Good coverage of edge cases:
model_context_window = 0should be rejected as invalidmodel_auto_compact_token_limit = "abc"(non-integer) should be rejected- Error messages in Chinese match the expected validation behavior
394-408: LGTM! Legacy config upgrade flow tests.Comprehensive coverage for the upgrade scenario:
- Status defaults are correctly restored (190000/185000)
get-config-templatewith empty params includes default values in TOML output- This ensures backward compatibility for existing configs without the new fields
tests/unit/config-tabs-ui.test.mjs (5)
32-41: LGTM! HTML assertions for context budget UI controls.Comprehensive validation of the new UI elements:
- Title span, v-model bindings for both inputs
- Blur and keydown.enter handlers for both fields
- Reset button with click handler and label text
- Grid container class
100-103: LGTM! Trash header layout refactoring assertions.The assertions correctly validate the updated trash header structure:
- New
.settings-tab-actions.trash-header-actionsclass combination- Compact tool buttons with proper click handlers
- Removal of the old title element (negative assertion)
201-208: LGTM! App.js state and handler assertions.Validates the new context budget wiring in app.js:
- Initial input values matching defaults (190000, 185000)
- Status response field mappings
- Blur handlers, reset function, and normalization helper
291-314: LGTM! Style assertions for trash item actions and codex config grid.The assertions validate:
- Updated
.trash-item-actionsgrid layout- Button sizing expectations
- Responsive rules for trash header actions
- New
.codex-config-gridand.codex-config-fieldstyle rules
320-343: LGTM! Trash header actions style assertions.Comprehensive validation of the new grid-based layout for
.trash-header-actions:
- Inline-grid display with column sizing
- Button alignment and sizing rules
- Hover state handling
- All expected CSS properties are verified
tests/unit/provider-share-command.test.mjs (4)
149-200: LGTM! Validation test for context budget parameters.Thorough unit test coverage:
- Tests both
model_context_window = 0andmodel_auto_compact_token_limit = "abc"rejection- Verifies exact error message objects
- Confirms
writeConfigis never called on validation failure
202-250: LGTM! Default restoration test for upgraded configs.Validates the upgrade path for legacy configs missing context budget fields:
- Mock config without budget keys
- Verifies generated template includes default values (190000, 185000)
- Uses proper regex assertions for TOML line format
252-279: LGTM! Config value reader fallback behavior test.Good edge case coverage:
- Missing keys return correct defaults (190000, 185000)
- Unknown keys return empty string
- Invalid value (
model_context_window: 0) returns empty string (not default)
285-371: LGTM! Re-entrant apply concurrency test.Comprehensive async test for the queued apply behavior:
- First apply starts and blocks on promise
- Second apply queues pending options
- After first resolves, pending options are merged and applied
- Verifies: 2 template requests, correct merged params, 2 loadAll calls, pending cleared, no messages with
silent: trueThis correctly validates the re-entrant apply handling described in the AI summary.
web-ui/app.js (1)
711-730: The new budget-field validation path is consistent.Status hydration, draft sanitization, blur normalization, and template payload generation all reuse the same positive-integer rules for the new context-budget inputs.
Also applies to: 3216-3278, 3357-3384
|
@coderabbitai 如果我直接合并会带来回归性异常吗 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/provider-share-command.test.mjs`:
- Around line 388-390: The test "status api case keeps lexical declarations
scoped to the switch branch" is brittle because it asserts an exact spacing
pattern for the switch case; update the assertion that inspects cliSource to use
a more flexible regex that tolerates arbitrary whitespace (e.g., use \s+ or \s*
between tokens and allow either single or double quotes) so the check still
matches the case 'status': { regardless of formatting changes while still
ensuring the case label and block brace exist.
🪄 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: 5a1c35b2-1c7a-4d27-b933-7179988d46b0
📒 Files selected for processing (2)
cli.jstests/unit/provider-share-command.test.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- cli.js
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-31T12:37:50.988Z
Learning: In PR `#59` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-30T04:47:15.339Z
Learning: In PR `#55` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: SurviveM
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-29T02:42:32.761Z
Learning: The repository maintainer SurviveM (GitHub: SurviveM, repo: SakuraByteCore/codexmate) prefers that CodeRabbit review suggestions do not introduce breaking changes. Reviews should focus on correctness, safety, and quality without recommending refactors or changes that would break existing APIs, interfaces, or behavior.
🔇 Additional comments (6)
tests/unit/provider-share-command.test.mjs (6)
149-200: Good guardrail: invalid context-budget values are rejected before any write.This test correctly verifies both error paths and ensures
writeConfig()is never called.
202-250: Default restoration coverage looks solid.The assertions for both
model_context_windowandmodel_auto_compact_token_limitdefaults are clear and match the intended upgrade behavior.
252-305: Explicit invalid-param rejection is well covered.These checks validate field-specific errors for both budget params and protect
getConfigTemplateinput validation behavior.
307-357: Great compatibility test for BOM + CRLF preservation.This catches a subtle file-format regression class while validating restored budget defaults.
359-387: Fallback semantics test is precise and useful.It correctly distinguishes “missing key” fallback from “present but invalid” behavior.
392-478: Re-entrant apply flow test is strong and behavior-focused.It verifies last-write-wins pending options, second apply execution, state reset, and silent-mode messaging correctly.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 5176 🏁 Script executed: Length of output: 4810
总体结论：直接合并的功能性回归风险较低，但有两处值得关注。 ✅ 低风险区域
|
|
@coderabbitai re-review |
|
Please limit all review feedback to correctness, safety, and quality only within the existing design. Do not suggest breaking changes, refactors, or alterations to existing APIs, interfaces, or behavior. 🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web-ui/app.js (1)
3446-3464:⚠️ Potential issue | 🟡 MinorSurface backend validation details on direct-apply failures.
Line 3454 and Line 3462 replace
tplRes.error/applyRes.errorwith generic toasts. With the new budget fields, that hides the actionable reason when the backend rejects a specific value combination. This path should mirroropenConfigTemplateEditor()and show the backend message when it exists.💡 Small non-breaking fix
if (tplRes.error) { - this.showMessage('获取模板失败', 'error'); + this.showMessage(tplRes.error || '获取模板失败', 'error'); return; } @@ if (applyRes.error) { - this.showMessage('应用模板失败', 'error'); + this.showMessage(applyRes.error || '应用模板失败', 'error'); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/app.js` around lines 3446 - 3464, The current direct-apply flow swallows backend validation errors by always showing generic toasts for tplRes.error and applyRes.error; change the error handling in the api('get-config-template') and api('apply-config-template') branches to mirror openConfigTemplateEditor(): when tplRes.error or applyRes.error exists, if the response contains a message or details from the backend include that text in the call to this.showMessage (fallback to the existing generic string if no backend message present), and avoid early-returning without surfacing the backend's validation message so users see the actionable reason.
♻️ Duplicate comments (2)
web-ui/styles.css (2)
1524-1532:⚠️ Potential issue | 🟠 Major小屏下回收站头部按钮仍可能横向溢出。
固定
96px列宽 +min-width: 96px会阻止按钮在窄容器中收缩；当前移动端规则只改高度，未放开宽度约束。💡 Non-breaking CSS fix
`@media` (max-width: 540px) { + .trash-header-actions { + width: 100%; + grid-auto-columns: minmax(0, 1fr); + } + .selector-header .trash-header-actions > .btn-tool, .selector-header .trash-header-actions > .btn-tool-compact { + width: 100%; + min-width: 0; height: 44px; min-height: 44px; } }Also applies to: 1534-1551, 4435-4439
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/styles.css` around lines 1524 - 1532, 在 .trash-header-actions 中当前使用固定 grid-auto-columns: 96px（及相关的 min-width）导致在窄屏时按钮水平溢出；将列宽约束放宽：替换固定列宽为可伸缩的列定义（例如使用可自适应的 minmax/auto 或移除 grid-auto-columns 并让项目自行收缩），并确保按钮项或容器允许收缩（例如为项目设置 min-width: 0 或移除强制 min-width），以便在小屏下按钮能换行或收缩；在修复时修改 .trash-header-actions（并同步到其他相同块：出现于注释提到的其他片段）以应用这一弹性宽度策略。
1709-1713:⚠️ Potential issue | 🟠 MajorCodex 预算网格在超窄宽度下存在溢出风险。
minmax(240px, 1fr)在窄容器中不能继续收缩，可能导致新输入区横向裁切。💡 Non-breaking CSS fix
.codex-config-grid { display: grid; gap: var(--spacing-sm); - grid-template-columns: repeat(auto-fit, minmax(240px, 1fr)); + grid-template-columns: repeat(auto-fit, minmax(min(240px, 100%), 1fr)); align-items: start; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/styles.css` around lines 1709 - 1713, The grid rule .codex-config-grid uses grid-template-columns: repeat(auto-fit, minmax(240px, 1fr)) which prevents columns from shrinking below 240px and causes overflow on very narrow viewports; update the grid-template-columns to use a flexible minimum (e.g., minmax(0, 1fr)) so items can shrink and avoid horizontal overflow, and if you still need a soft minimum width apply a max-width or padding on the child elements (adjust selectors that render the input area) rather than locking the grid column min size.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e/test-config.js`:
- Around line 91-99: The test only asserts injected budget lines but must also
verify the returned template still contains the base provider/model entries;
update the assertions around the api('get-config-template') call
(templateContextBudget) to additionally assert that
templateContextBudget.template includes model_provider and model (e.g., regex
checks for lines like model_provider = 'shadow' and model = 'shadow-model') so
the test fails if get-config-template drops those core entries when budget
params are present.
In `@tests/unit/config-tabs-ui.test.mjs`:
- Around line 39-40: The two assertions are independent and allow different
elements to satisfy the attribute and the label; instead, scope the check to a
single element by asserting that an element (e.g., a <button>) both has the
`@click` binding resetCodexContextBudgetDefaults and contains the label "重置默认值".
Update the test around resetCodexContextBudgetDefaults to either query the
rendered DOM for a button element with the `@click` attribute and assert its
textContent equals/contains "重置默认值", or use a single regex that matches a button
tag containing `@click`="resetCodexContextBudgetDefaults" and the "重置默认值" text
together.
In `@tests/unit/provider-share-command.test.mjs`:
- Around line 470-477: Add an assertion that the second apply uses the refreshed
template by checking appliedTemplates[1] matches the second template returned by
the second get-config-template request (e.g.,
assert.strictEqual(appliedTemplates[1].templateId || appliedTemplates[1].id ||
appliedTemplates[1].name, templateRequests[1].id || templateRequests[1].name) or
otherwise compare the appliedTemplates[1] payload to templateRequests[1]) so the
test fails if the replayed apply still uses template-1.
In `@web-ui/app.js`:
- Around line 711-730: When reloading status in loadAll() you unconditionally
overwrite input drafts (modelContextWindowInput,
modelAutoCompactTokenLimitInput) which clobbers an in-flight edit; change the
update logic so loadAll() only assigns these inputs when the user is not
actively editing them (e.g., check DOM focus or an
"isEditingModelContextWindow"/"isEditingModelAutoCompactTokenLimit" flag set on
focus/blur), or only replace the input when the current value still equals the
last-known server value (no local edits). Apply the same guard wherever status
reloads replace draft inputs so actively edited fields are preserved across the
in-flight apply.
---
Outside diff comments:
In `@web-ui/app.js`:
- Around line 3446-3464: The current direct-apply flow swallows backend
validation errors by always showing generic toasts for tplRes.error and
applyRes.error; change the error handling in the api('get-config-template') and
api('apply-config-template') branches to mirror openConfigTemplateEditor(): when
tplRes.error or applyRes.error exists, if the response contains a message or
details from the backend include that text in the call to this.showMessage
(fallback to the existing generic string if no backend message present), and
avoid early-returning without surfacing the backend's validation message so
users see the actionable reason.
---
Duplicate comments:
In `@web-ui/styles.css`:
- Around line 1524-1532: 在 .trash-header-actions 中当前使用固定 grid-auto-columns:
96px（及相关的 min-width）导致在窄屏时按钮水平溢出；将列宽约束放宽：替换固定列宽为可伸缩的列定义（例如使用可自适应的 minmax/auto
或移除 grid-auto-columns 并让项目自行收缩），并确保按钮项或容器允许收缩（例如为项目设置 min-width: 0 或移除强制
min-width），以便在小屏下按钮能换行或收缩；在修复时修改
.trash-header-actions（并同步到其他相同块：出现于注释提到的其他片段）以应用这一弹性宽度策略。
- Around line 1709-1713: The grid rule .codex-config-grid uses
grid-template-columns: repeat(auto-fit, minmax(240px, 1fr)) which prevents
columns from shrinking below 240px and causes overflow on very narrow viewports;
update the grid-template-columns to use a flexible minimum (e.g., minmax(0,
1fr)) so items can shrink and avoid horizontal overflow, and if you still need a
soft minimum width apply a max-width or padding on the child elements (adjust
selectors that render the input area) rather than locking the grid column min
size.
🪄 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: 0427f2fb-4aab-4a12-b3c5-c48dcef1533f
📒 Files selected for processing (7)
cli.jstests/e2e/test-config.jstests/unit/config-tabs-ui.test.mjstests/unit/provider-share-command.test.mjsweb-ui/app.jsweb-ui/index.htmlweb-ui/styles.css
📜 Review details
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-31T12:37:50.988Z
Learning: In PR `#59` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-30T04:47:15.339Z
Learning: In PR `#55` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: SurviveM
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-29T02:42:32.761Z
Learning: The repository maintainer SurviveM (GitHub: SurviveM, repo: SakuraByteCore/codexmate) prefers that CodeRabbit review suggestions do not introduce breaking changes. Reviews should focus on correctness, safety, and quality without recommending refactors or changes that would break existing APIs, interfaces, or behavior.
📚 Learning: 2026-03-23T15:38:41.305Z
Learnt from: ymkiux
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-23T15:38:41.305Z
Learning: In the SakuraByteCore/codexmate repository, BOM (U+FEFF) characters in Chinese markdown and JavaScript config files (e.g., site/index.md, site/.vitepress/config.js) are intentional and follow the project's encoding convention. Do not flag BOM presence in these files as an issue. The PostCSS BOM parse issue was addressed separately by adding BOM-free site/package.json and site/postcss.config.cjs files.
Applied to files:
cli.js
🔇 Additional comments (10)
web-ui/index.html (2)
471-517: 上下文预算设置区的交互闭环完整。输入清洗、失焦/回车提交、以及应用中禁用态都已覆盖，和现有配置写入流程一致。
1171-1178: 回收站操作区的类名对齐正确。
trash-header-actions与通用settings-tab-actions组合使用，便于样式复用且不改变原有行为。web-ui/styles.css (2)
1508-1510: 设置头部对齐策略与新结构一致。该调整与回收站头部仅保留操作按钮的结构匹配，没有引入行为变更。
2144-2158: 回收站卡片操作按钮的栅格化处理合理。按钮统一占满单元格并居中，交互面积和视觉对齐都更稳定。
web-ui/app.js (2)
3216-3244: Validation is nicely centralized here.Both budget fields now share one sanitizer/normalizer, and
Number.isSafeIntegerkeeps malformed or out-of-range values from reaching the API.
3405-3412: The pending-apply replay closes the dropped-update window.Merging
_pendingCodexApplyOptionswhilethis.codexApplyingis true and replaying them infinallykeeps later blur/reset changes from being silently discarded.Also applies to: 3475-3480
cli.js (4)
96-97: Defaults are aligned across the generated config paths.Both the virtual template and the managed bootstrap config now emit the same context-budget keys, which keeps fresh installs, resets, and template generation in sync.
Also applies to: 231-232, 3175-3176
3342-3360: The budget validation now fails fast on both generation and apply paths.Bad explicit values are surfaced as errors instead of being silently ignored or persisted into
config.toml.Also applies to: 3391-3441, 3473-3485
3362-3379: Nice: BOM and line-ending preservation are handled during insertion.That avoids unintended format churn when legacy templates are updated with the new keys.
11692-11710: Verify omitted workflow inputs stayundefined.These new optional fields are forwarded straight into
getConfigTemplate(). If the workflow templater materializes a missing{{input.*}}as''rather thanundefined, the new positive-integer checks will rejectsafe-provider-switcheven when callers omit both fields.
|
@coderabbitai re-review |
|
Re-review PR 🧠 Learnings used✅ Actions performedFull review triggered. |
|
@coderabbitai re-review |
|
Please limit all review feedback to correctness, safety, and quality only within the existing design. Do not suggest breaking changes, refactors, or alterations to existing APIs, interfaces, or behavior. 🧠 Learnings used🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
cli.js (1)
3447-3458:⚠️ Potential issue | 🟡 MinorConfig load failures still synthesize budget defaults.
When
readConfigOrVirtualDefault()falls back to{}after a read/parse failure, this helper still returns190000/185000, so/api statusand MCP status report invented values even though the config did not load.💡 Minimal fix
-function readPositiveIntegerConfigValue(config, key) { +function readPositiveIntegerConfigValue(config, key, options = {}) { + const useDefaultWhenMissing = options.useDefaultWhenMissing !== false; if (!config || typeof config !== 'object' || !key) { return ''; } const raw = config[key]; - if (raw === undefined) { + if (raw === undefined && useDefaultWhenMissing) { if (key === 'model_context_window') return DEFAULT_MODEL_CONTEXT_WINDOW; if (key === 'model_auto_compact_token_limit') return DEFAULT_MODEL_AUTO_COMPACT_TOKEN_LIMIT; } const normalized = normalizePositiveIntegerParam(raw); return normalized === null ? '' : normalized; }Then pass
useDefaultWhenMissing: falsefrom the Web/API and MCP status builders when the config load failed.Also applies to: 10093-10113, 11586-11599
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 3447 - 3458, The helper readPositiveIntegerConfigValue currently synthesizes hard defaults (DEFAULT_MODEL_CONTEXT_WINDOW, DEFAULT_MODEL_AUTO_COMPACT_TOKEN_LIMIT) when the requested key is missing, causing status endpoints to report invented budgets after readConfigOrVirtualDefault() falls back to {}. Modify readPositiveIntegerConfigValue to accept an optional flag (e.g., useDefaultWhenMissing = true) and when useDefaultWhenMissing is false and raw === undefined return '' (no default); update callers (the Web/API and MCP status builders that call readConfigOrVirtualDefault() when a config load failed) to pass useDefaultWhenMissing: false so no defaults are synthesized in failure cases. Ensure existing callers that expect current behavior keep the default flag true.tests/e2e/test-config.js (1)
97-99:⚠️ Potential issue | 🟡 MinorKeep base provider/model assertions in this context-budget template case.
Line 97-Line 99 only verifies injected budget keys. The test can still pass if
model_provider/modelare accidentally dropped from the returned template.✅ Minimal test hardening
assert(typeof templateContextBudget.template === 'string', 'get-config-template(context budget) missing template'); + assert(templateContextBudget.template.includes('model_provider = "shadow"'), 'get-config-template(context budget) missing provider override'); + assert(templateContextBudget.template.includes('model = "shadow-model"'), 'get-config-template(context budget) missing model override'); assert(/^\s*model_context_window\s*=\s*200000\s*$/m.test(templateContextBudget.template), 'get-config-template(context budget) missing model_context_window'); assert(/^\s*model_auto_compact_token_limit\s*=\s*195000\s*$/m.test(templateContextBudget.template), 'get-config-template(context budget) missing model_auto_compact_token_limit');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/test-config.js` around lines 97 - 99, The test only checks injected budget keys and may pass if model_provider/model are removed; update tests/e2e/test-config.js (around the templateContextBudget assertions) to also assert that templateContextBudget.template contains the base provider and model entries by adding explicit checks for model_provider and model (e.g., assert(/\bmodel_provider\s*=/ or similar and assert(/\bmodel\s*=/) or simple typeof/string presence checks) so the template must include both fields in addition to the existing budget-key assertions.tests/unit/provider-share-command.test.mjs (1)
470-474:⚠️ Potential issue | 🟡 MinorStill assert that the replayed apply uses the refreshed template.
These assertions prove a second template fetch happens, but they still do not prove the second
apply-config-templatecall usestemplate-2. Re-applyingtemplate-1would pass here.🧪 Small assertion addition
assert.strictEqual(templateRequests.length, 2); assert.strictEqual(templateRequests[1].modelContextWindow, 190000); assert.strictEqual(templateRequests[1].modelAutoCompactTokenLimit, 175000); assert.strictEqual(appliedTemplates.length, 2); + assert.strictEqual(appliedTemplates[0].template, 'template-1'); + assert.strictEqual(appliedTemplates[1].template, 'template-2'); assert.strictEqual(loadAllCalls, 2);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/provider-share-command.test.mjs` around lines 470 - 474, Add an assertion that the second applied template is the refreshed template (template-2) so the replayed apply-config-template call actually used the new template; specifically, after the existing checks for templateRequests and appliedTemplates, assert a unique identifier/property on appliedTemplates[1] (for example .id or .name) equals the expected template-2 value (or compare appliedTemplates[1] to templateRequests[1] if the test stores template-2), ensuring the second apply-config-template invocation used the refreshed template rather than re-applying template-1.tests/unit/config-tabs-ui.test.mjs (1)
39-40:⚠️ Potential issue | 🟡 MinorScope the reset handler and label to the same button.
These two regexes can be satisfied by different nodes, so a miswired reset control still passes.
🧪 Make the assertion element-scoped
- assert.match(html, /@click="resetCodexContextBudgetDefaults"/); - assert.match(html, />\s*重置默认值\s*<\/button>/); + assert.match( + html, + /<button[^>]*@click="resetCodexContextBudgetDefaults"[^>]*>[\s\S]*?重置默认值[\s\S]*?<\/button>/ + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/config-tabs-ui.test.mjs` around lines 39 - 40, The two separate regex assertions can match different nodes; update the test in tests/unit/config-tabs-ui.test.mjs so the reset handler and label are asserted on the same button element: locate the button element with the `@click`="resetCodexContextBudgetDefaults" handler and assert that that same element's inner text equals/contains "重置默认值" (or assert a single combined pattern that matches a <button> element having the `@click` handler and the "重置默认值" label). This ensures resetCodexContextBudgetDefaults and the "重置默认值" label are scoped to the same DOM node.web-ui/styles.css (1)
1535-1546:⚠️ Potential issue | 🟡 MinorThese 32px rules still defeat the force-compact touch target.
Earlier
body.force-compact .btn-toolraises controls to 44px, but this more specific selector resets bothheightandmin-heightto32px. Wheneverbody.force-compactis active outside themax-width: 540pxoverride, the trash-header actions shrink again.💡 Non-breaking CSS fix
+body.force-compact .selector-header .trash-header-actions > .btn-tool, +body.force-compact .selector-header .trash-header-actions > .btn-tool-compact { + height: 44px; + min-height: 44px; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/styles.css` around lines 1535 - 1546, The current rules for .selector-header .trash-header-actions > .btn-tool and .btn-tool-compact forcibly set height and min-height to 32px, which overrides the body.force-compact touch-target rule; remove the fixed height (and/or the min-height:32px) from that selector so it no longer forces 32px, and instead rely on min-height only or add a more specific override such as body.force-compact .selector-header .trash-header-actions > .btn-tool(.btn-tool-compact) { min-height: 44px; } so the force-compact state can raise controls to 44px.web-ui/app.js (1)
711-730:⚠️ Potential issue | 🟠 MajorPreserve unsaved draft values during status reload.
Line 717 and Line 727 still overwrite local drafts unconditionally. If one field blurs/applies while the user is typing in the sibling field,
loadAll()can clobber that in-progress draft.💡 Non-breaking guard (only sync when draft is unchanged since last server sync)
@@ data() { return { + _lastSyncedModelContextWindowInput: '190000', + _lastSyncedModelAutoCompactTokenLimitInput: '185000', @@ async loadAll() { - this.modelContextWindowInput = contextWindow.ok && contextWindow.text - ? contextWindow.text - : '190000'; + const nextContextWindowText = contextWindow.ok && contextWindow.text + ? contextWindow.text + : '190000'; + if (this.modelContextWindowInput === this._lastSyncedModelContextWindowInput || !this.modelContextWindowInput) { + this.modelContextWindowInput = nextContextWindowText; + } + this._lastSyncedModelContextWindowInput = nextContextWindowText; @@ - this.modelAutoCompactTokenLimitInput = autoCompactTokenLimit.ok && autoCompactTokenLimit.text - ? autoCompactTokenLimit.text - : '185000'; + const nextAutoCompactText = autoCompactTokenLimit.ok && autoCompactTokenLimit.text + ? autoCompactTokenLimit.text + : '185000'; + if (this.modelAutoCompactTokenLimitInput === this._lastSyncedModelAutoCompactTokenLimitInput || !this.modelAutoCompactTokenLimitInput) { + this.modelAutoCompactTokenLimitInput = nextAutoCompactText; + } + this._lastSyncedModelAutoCompactTokenLimitInput = nextAutoCompactText;Based on learnings: the maintainer requires non-breaking suggestions focused on correctness/safety/quality within the current design.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/app.js` around lines 711 - 730, The reload currently unconditionally overwrites in-progress drafts for modelContextWindowInput and modelAutoCompactTokenLimitInput; modify the logic in the reload (where normalizePositiveIntegerInput is used) to only assign the server-derived text to this.modelContextWindowInput and this.modelAutoCompactTokenLimitInput if the current draft is unchanged since the last server sync—introduce per-field last-synced trackers (e.g., this.modelContextWindowLastSynced and this.modelAutoCompactTokenLimitLastSynced), update those trackers when you accept a server value, and before assigning check that the current input equals the tracker (or is empty) so active user edits are not clobbered; continue to use normalizePositiveIntegerInput to compute the candidate text and only replace the input when the “unchanged since last sync” guard passes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/provider-share-command.test.mjs`:
- Around line 160-175: The test currently only asserts writeConfig() is not
called on validation failure; extend the no-side-effects check by making the
other side-effecting stubs (updateAuthJson, writeModels, writeCurrentModels,
recordRecentConfig) either increment a counter or throw if invoked and then
assert they were not called; update the instantiation block where
applyConfigTemplate is instantiated (the functions named updateAuthJson,
writeModels, writeCurrentModels, recordRecentConfig) to use these guards so the
test fails if any of them run during the integer validation failure.
---
Duplicate comments:
In `@cli.js`:
- Around line 3447-3458: The helper readPositiveIntegerConfigValue currently
synthesizes hard defaults (DEFAULT_MODEL_CONTEXT_WINDOW,
DEFAULT_MODEL_AUTO_COMPACT_TOKEN_LIMIT) when the requested key is missing,
causing status endpoints to report invented budgets after
readConfigOrVirtualDefault() falls back to {}. Modify
readPositiveIntegerConfigValue to accept an optional flag (e.g.,
useDefaultWhenMissing = true) and when useDefaultWhenMissing is false and raw
=== undefined return '' (no default); update callers (the Web/API and MCP status
builders that call readConfigOrVirtualDefault() when a config load failed) to
pass useDefaultWhenMissing: false so no defaults are synthesized in failure
cases. Ensure existing callers that expect current behavior keep the default
flag true.
In `@tests/e2e/test-config.js`:
- Around line 97-99: The test only checks injected budget keys and may pass if
model_provider/model are removed; update tests/e2e/test-config.js (around the
templateContextBudget assertions) to also assert that
templateContextBudget.template contains the base provider and model entries by
adding explicit checks for model_provider and model (e.g.,
assert(/\bmodel_provider\s*=/ or similar and assert(/\bmodel\s*=/) or simple
typeof/string presence checks) so the template must include both fields in
addition to the existing budget-key assertions.
In `@tests/unit/config-tabs-ui.test.mjs`:
- Around line 39-40: The two separate regex assertions can match different
nodes; update the test in tests/unit/config-tabs-ui.test.mjs so the reset
handler and label are asserted on the same button element: locate the button
element with the `@click`="resetCodexContextBudgetDefaults" handler and assert
that that same element's inner text equals/contains "重置默认值" (or assert a single
combined pattern that matches a <button> element having the `@click` handler and
the "重置默认值" label). This ensures resetCodexContextBudgetDefaults and the "重置默认值"
label are scoped to the same DOM node.
In `@tests/unit/provider-share-command.test.mjs`:
- Around line 470-474: Add an assertion that the second applied template is the
refreshed template (template-2) so the replayed apply-config-template call
actually used the new template; specifically, after the existing checks for
templateRequests and appliedTemplates, assert a unique identifier/property on
appliedTemplates[1] (for example .id or .name) equals the expected template-2
value (or compare appliedTemplates[1] to templateRequests[1] if the test stores
template-2), ensuring the second apply-config-template invocation used the
refreshed template rather than re-applying template-1.
In `@web-ui/app.js`:
- Around line 711-730: The reload currently unconditionally overwrites
in-progress drafts for modelContextWindowInput and
modelAutoCompactTokenLimitInput; modify the logic in the reload (where
normalizePositiveIntegerInput is used) to only assign the server-derived text to
this.modelContextWindowInput and this.modelAutoCompactTokenLimitInput if the
current draft is unchanged since the last server sync—introduce per-field
last-synced trackers (e.g., this.modelContextWindowLastSynced and
this.modelAutoCompactTokenLimitLastSynced), update those trackers when you
accept a server value, and before assigning check that the current input equals
the tracker (or is empty) so active user edits are not clobbered; continue to
use normalizePositiveIntegerInput to compute the candidate text and only replace
the input when the “unchanged since last sync” guard passes.
In `@web-ui/styles.css`:
- Around line 1535-1546: The current rules for .selector-header
.trash-header-actions > .btn-tool and .btn-tool-compact forcibly set height and
min-height to 32px, which overrides the body.force-compact touch-target rule;
remove the fixed height (and/or the min-height:32px) from that selector so it no
longer forces 32px, and instead rely on min-height only or add a more specific
override such as body.force-compact .selector-header .trash-header-actions >
.btn-tool(.btn-tool-compact) { min-height: 44px; } so the force-compact state
can raise controls to 44px.
🪄 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: db5bbb2d-6614-4289-b8d1-bde35498e4c2
📒 Files selected for processing (7)
cli.jstests/e2e/test-config.jstests/unit/config-tabs-ui.test.mjstests/unit/provider-share-command.test.mjsweb-ui/app.jsweb-ui/index.htmlweb-ui/styles.css
📜 Review details
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-31T12:37:50.988Z
Learning: In PR `#59` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-30T04:47:15.339Z
Learning: In PR `#55` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: SurviveM
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-29T02:42:32.761Z
Learning: The repository maintainer SurviveM (GitHub: SurviveM, repo: SakuraByteCore/codexmate) prefers that CodeRabbit review suggestions do not introduce breaking changes. Reviews should focus on correctness, safety, and quality without recommending refactors or changes that would break existing APIs, interfaces, or behavior.
📚 Learning: 2026-03-23T15:38:41.305Z
Learnt from: ymkiux
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-23T15:38:41.305Z
Learning: In the SakuraByteCore/codexmate repository, BOM (U+FEFF) characters in Chinese markdown and JavaScript config files (e.g., site/index.md, site/.vitepress/config.js) are intentional and follow the project's encoding convention. Do not flag BOM presence in these files as an issue. The PostCSS BOM parse issue was addressed separately by adding BOM-free site/package.json and site/postcss.config.cjs files.
Applied to files:
cli.js
🔇 Additional comments (15)
cli.js (4)
96-97: Defaults stay aligned across all bootstrap paths.Reusing the same constants in both virtual and persisted templates keeps fresh installs, resets, and generated defaults in sync.
Also applies to: 231-232, 3175-3176
3342-3360: Explicit budget inputs now fail in the right places.The omitted-vs-invalid split is consistent in template generation and apply-time parsing, so bad explicit values are rejected instead of being silently ignored.
Also applies to: 3391-3441, 3473-3485
3362-3379: Format preservation looks good here.Keeping the original BOM and detected line ending while reinserting the keys avoids needless config churn on legacy files.
11692-11693: Workflow/MCP wiring stays consistent.Using the same camelCase field names across workflow input schema, workflow step arguments, and MCP tool input schema should keep UI, workflow, and MCP callers aligned.
Also applies to: 11708-11710, 12279-12289
web-ui/index.html (2)
471-517: Looks good: context-budget UI wiring is coherent and guarded.The new inputs are properly labeled, sanitize on input, and commit on blur/Enter as intended. Reset action also respects loading/apply/error guard states.
1171-1179: Layout consolidation here is clean and behavior-safe.The action container simplification in the trash header is straightforward and keeps control semantics intact.
tests/e2e/test-config.js (4)
25-28: Good status-contract coverage for new defaults.These assertions correctly lock presence and default values for both new context-budget status fields.
106-129: Combined template-path assertions are well-covered.Including both context-budget keys in combined/high-effort template scenarios improves regression detection across option combinations.
130-159: Validation-path tests are solid additions.The two negative
apply-config-templatecases strengthen server-side guard coverage for non-positive and non-integer context-budget values.
394-408: Legacy compatibility checks are valuable and correctly scoped.These assertions protect default backfill behavior for both status payload and template generation on legacy config bootstrap paths.
web-ui/app.js (5)
57-58: Additive defaults/state wiring looks good.The new defaults and queued-apply placeholder are introduced in a backward-compatible way and align with the existing state model.
Also applies to: 119-120, 157-157
3216-3244: Validation helpers are solid and reusable.The draft sanitization + normalization split is clean, and the positive-integer checks are consistent.
3246-3287: Blur/reset apply flow is correctly implemented.Good sequencing: normalize → surface error if invalid → write normalized text → apply explicit overrides.
3358-3384: Template flow now forwards validated budget fields correctly.Both context budget inputs are validated before request and passed explicitly in
get-config-template.
3406-3412: Queued re-entrant apply handling looks correct.Merging pending options while
codexApplyingand draining infinallyis a good reliability fix and keeps latest edits from being dropped.Also applies to: 3421-3443, 3450-3453, 3476-3480
|
@coderabbitai re-review |
|
Please limit all review feedback to correctness, safety, and quality only within the existing design. Do not suggest breaking changes, refactors, or alterations to existing APIs, interfaces, or behavior. 🧠 Learnings used✅ Actions performedFull review triggered. |
|
@coderabbitai re-review |
|
Please limit all review feedback to correctness, safety, and quality only within the existing design. Do not suggest breaking changes, refactors, or alterations to existing APIs, interfaces, or behavior. 🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
♻️ Duplicate comments (4)
tests/unit/provider-share-command.test.mjs (1)
160-175:⚠️ Potential issue | 🟡 MinorValidation-failure coverage still misses the other write paths.
This only proves
writeConfig()stays at 0. IfapplyConfigTemplate()starts callingupdateAuthJson(),writeModels(),writeCurrentModels(), orrecordRecentConfig()before returning the integer-validation error, this test will still pass while leaving partial state behind.🧪 Tighten the no-side-effects assertion
const applyConfigTemplateSource = extractBlockBySignature(cliSource, 'function applyConfigTemplate(params = {}) {'); let writeConfigCalls = 0; + let updateAuthJsonCalls = 0; + let writeModelsCalls = 0; + let writeCurrentModelsCalls = 0; + let recordRecentConfigCalls = 0; const applyConfigTemplate = instantiateFunction(applyConfigTemplateSource, 'applyConfigTemplate', { toml: require('@iarna/toml'), normalizePositiveIntegerParam, writeConfig() { writeConfigCalls += 1; }, - updateAuthJson() {}, + updateAuthJson() { + updateAuthJsonCalls += 1; + }, readModels() { return []; }, - writeModels() {}, + writeModels() { + writeModelsCalls += 1; + }, readCurrentModels() { return {}; }, - writeCurrentModels() {}, - recordRecentConfig() {} + writeCurrentModels() { + writeCurrentModelsCalls += 1; + }, + recordRecentConfig() { + recordRecentConfigCalls += 1; + } }); @@ assert.deepStrictEqual(invalidContextResult, { error: '模板中的 model_context_window 必须是正整数' }); assert.deepStrictEqual(invalidAutoCompactResult, { error: '模板中的 model_auto_compact_token_limit 必须是正整数' }); assert.strictEqual(writeConfigCalls, 0); + assert.strictEqual(updateAuthJsonCalls, 0); + assert.strictEqual(writeModelsCalls, 0); + assert.strictEqual(writeCurrentModelsCalls, 0); + assert.strictEqual(recordRecentConfigCalls, 0);Also applies to: 197-199
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/provider-share-command.test.mjs` around lines 160 - 175, The test only checks writeConfigCalls; extend the no-side-effects assertions by instrumenting all mutating callbacks used when instantiating applyConfigTemplate—specifically updateAuthJson, writeModels, writeCurrentModels, and recordRecentConfig—so each increments its own counter (or a shared counter with distinct labels) just like writeConfig does, then assert all of those counters remain zero after calling applyConfigTemplate; ensure the instantiated function call in the test uses these instrumented stubs and add assertions for their counters (in addition to writeConfigCalls) to guarantee no other write paths ran before the integer-validation error is returned.web-ui/styles.css (1)
1535-1546:⚠️ Potential issue | 🟠 Major
32pxtrash-header buttons still override the touch-mode 44px fallback.Line 1545 and Line 1546 are more specific than the earlier
body.force-compact .btn-tool { min-height: 44px; }rule, so tablets/touch laptops that enterforce-compactabove 540px still get 32px tap targets here.💡 Small non-breaking CSS fix
+body.force-compact .selector-header .trash-header-actions > .btn-tool, +body.force-compact .selector-header .trash-header-actions > .btn-tool-compact { + height: 44px; + min-height: 44px; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/styles.css` around lines 1535 - 1546, The .selector-header .trash-header-actions > .btn-tool and .btn-tool-compact rules hard-set height/min-height to 32px which overrides the touch-mode fallback (body.force-compact .btn-tool { min-height: 44px }); update those selectors (selector-header .trash-header-actions > .btn-tool and .btn-tool-compact) to remove the fixed height and 32px min-height and instead use height: auto (or no height) and min-height: 44px so the body.force-compact touch target size is honored on tablets/touch laptops.web-ui/app.js (1)
3251-3266:⚠️ Potential issue | 🟠 MajorPreserve the edit guard for Enter-triggered applies.
Line 3252 and Line 3270 clear
editingCodexBudgetFieldeven when these handlers run from the inputs’@keydown.enterbindings. Enter does not blur the field, so when Line 3490 reloads state, Lines 718-731 can treat the still-focused input as idle and overwrite a newer draft the user starts typing before the request finishes.💡 Non-breaking fix
- async onModelContextWindowBlur() { - this.editingCodexBudgetField = ''; + async onModelContextWindowBlur(event) { + this.editingCodexBudgetField = event && event.type === 'keydown' + ? 'modelContextWindowInput' + : ''; const normalized = this.normalizePositiveIntegerInput( this.modelContextWindowInput, 'model_context_window', DEFAULT_MODEL_CONTEXT_WINDOW ); @@ - async onModelAutoCompactTokenLimitBlur() { - this.editingCodexBudgetField = ''; + async onModelAutoCompactTokenLimitBlur(event) { + this.editingCodexBudgetField = event && event.type === 'keydown' + ? 'modelAutoCompactTokenLimitInput' + : ''; const normalized = this.normalizePositiveIntegerInput( this.modelAutoCompactTokenLimitInput, 'model_auto_compact_token_limit', DEFAULT_MODEL_AUTO_COMPACT_TOKEN_LIMIT );Also applies to: 3269-3284
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/app.js` around lines 3251 - 3266, The blur handlers (e.g., onModelContextWindowBlur and the similar handlers around lines 3269-3284) are clearing editingCodexBudgetField even when invoked from Enter-key handlers, overwriting an in-progress draft; modify each blur handler to first detect whether the input is still focused (e.g., compare document.activeElement to the relevant input ref or validate focus) and if so, skip clearing editingCodexBudgetField and/or skip the remainder of the blur logic; keep calling applyCodexConfigDirect(...) as before but only clear editingCodexBudgetField when the element truly lost focus (not when the handler was triggered by an Enter key path).cli.js (1)
3475-3487:⚠️ Potential issue | 🟠 MajorReject impossible budget pairs before saving.
applyConfigTemplate()validates the two fields independently, so a template can still persistmodel_auto_compact_token_limitabovemodel_context_window. That leaves a compact threshold outside the configured window and saves a self-contradictory budget pair.💡 Minimal guard
+ const normalizedModelContextWindow = Object.prototype.hasOwnProperty.call(parsed, 'model_context_window') + ? normalizePositiveIntegerParam(parsed.model_context_window) + : null; + const normalizedModelAutoCompactTokenLimit = Object.prototype.hasOwnProperty.call(parsed, 'model_auto_compact_token_limit') + ? normalizePositiveIntegerParam(parsed.model_auto_compact_token_limit) + : null; + if ( Object.prototype.hasOwnProperty.call(parsed, 'model_context_window') - && normalizePositiveIntegerParam(parsed.model_context_window) === null + && normalizedModelContextWindow === null ) { return { error: '模板中的 model_context_window 必须是正整数' }; } if ( Object.prototype.hasOwnProperty.call(parsed, 'model_auto_compact_token_limit') - && normalizePositiveIntegerParam(parsed.model_auto_compact_token_limit) === null + && normalizedModelAutoCompactTokenLimit === null ) { return { error: '模板中的 model_auto_compact_token_limit 必须是正整数' }; } + + if ( + normalizedModelContextWindow !== null + && normalizedModelAutoCompactTokenLimit !== null + && normalizedModelAutoCompactTokenLimit > normalizedModelContextWindow + ) { + return { error: '模板中的 model_auto_compact_token_limit 不能大于 model_context_window' }; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 3475 - 3487, applyConfigTemplate currently validates model_context_window and model_auto_compact_token_limit independently, allowing model_auto_compact_token_limit to be larger than model_context_window; add a cross-field guard after both individual validations: call normalizePositiveIntegerParam on parsed.model_context_window and parsed.model_auto_compact_token_limit, and if both return non-null and model_auto_compact_token_limit > model_context_window return an error (e.g., "模板中的 model_auto_compact_token_limit 不能大于 model_context_window"); update the early-return logic in applyConfigTemplate to perform this comparison after the existing per-field checks so contradictory budget pairs are rejected before saving.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cli.js`:
- Around line 3475-3487: applyConfigTemplate currently validates
model_context_window and model_auto_compact_token_limit independently, allowing
model_auto_compact_token_limit to be larger than model_context_window; add a
cross-field guard after both individual validations: call
normalizePositiveIntegerParam on parsed.model_context_window and
parsed.model_auto_compact_token_limit, and if both return non-null and
model_auto_compact_token_limit > model_context_window return an error (e.g.,
"模板中的 model_auto_compact_token_limit 不能大于 model_context_window"); update the
early-return logic in applyConfigTemplate to perform this comparison after the
existing per-field checks so contradictory budget pairs are rejected before
saving.
In `@tests/unit/provider-share-command.test.mjs`:
- Around line 160-175: The test only checks writeConfigCalls; extend the
no-side-effects assertions by instrumenting all mutating callbacks used when
instantiating applyConfigTemplate—specifically updateAuthJson, writeModels,
writeCurrentModels, and recordRecentConfig—so each increments its own counter
(or a shared counter with distinct labels) just like writeConfig does, then
assert all of those counters remain zero after calling applyConfigTemplate;
ensure the instantiated function call in the test uses these instrumented stubs
and add assertions for their counters (in addition to writeConfigCalls) to
guarantee no other write paths ran before the integer-validation error is
returned.
In `@web-ui/app.js`:
- Around line 3251-3266: The blur handlers (e.g., onModelContextWindowBlur and
the similar handlers around lines 3269-3284) are clearing
editingCodexBudgetField even when invoked from Enter-key handlers, overwriting
an in-progress draft; modify each blur handler to first detect whether the input
is still focused (e.g., compare document.activeElement to the relevant input ref
or validate focus) and if so, skip clearing editingCodexBudgetField and/or skip
the remainder of the blur logic; keep calling applyCodexConfigDirect(...) as
before but only clear editingCodexBudgetField when the element truly lost focus
(not when the handler was triggered by an Enter key path).
In `@web-ui/styles.css`:
- Around line 1535-1546: The .selector-header .trash-header-actions > .btn-tool
and .btn-tool-compact rules hard-set height/min-height to 32px which overrides
the touch-mode fallback (body.force-compact .btn-tool { min-height: 44px });
update those selectors (selector-header .trash-header-actions > .btn-tool and
.btn-tool-compact) to remove the fixed height and 32px min-height and instead
use height: auto (or no height) and min-height: 44px so the body.force-compact
touch target size is honored on tablets/touch laptops.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dd8d94cd-18ba-4d65-8d7e-8b7b8b171b94
📒 Files selected for processing (7)
cli.jstests/e2e/test-config.jstests/unit/config-tabs-ui.test.mjstests/unit/provider-share-command.test.mjsweb-ui/app.jsweb-ui/index.htmlweb-ui/styles.css
📜 Review details
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-31T12:37:50.988Z
Learning: In PR `#59` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-30T04:47:15.339Z
Learning: In PR `#55` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: SurviveM
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-29T02:42:32.761Z
Learning: The repository maintainer SurviveM (GitHub: SurviveM, repo: SakuraByteCore/codexmate) prefers that CodeRabbit review suggestions do not introduce breaking changes. Reviews should focus on correctness, safety, and quality without recommending refactors or changes that would break existing APIs, interfaces, or behavior.
📚 Learning: 2026-03-23T15:38:41.305Z
Learnt from: ymkiux
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-23T15:38:41.305Z
Learning: In the SakuraByteCore/codexmate repository, BOM (U+FEFF) characters in Chinese markdown and JavaScript config files (e.g., site/index.md, site/.vitepress/config.js) are intentional and follow the project's encoding convention. Do not flag BOM presence in these files as an issue. The PostCSS BOM parse issue was addressed separately by adding BOM-free site/package.json and site/postcss.config.cjs files.
Applied to files:
cli.js
🔇 Additional comments (8)
web-ui/index.html (2)
471-519: Context-budget input section wiring looks consistent and safe.The new fields, sanitize/commit handlers, and reset action guards are coherent with the existing flow and introduce no correctness blockers in this segment.
1173-1180: Trash header action class update is clean.This class update is behavior-preserving and aligns with the intended layout hook without introducing functional risk.
tests/e2e/test-config.js (4)
25-28: Status payload coverage for new budget fields is solid.These assertions correctly lock in presence and expected default values for the newly introduced status fields.
90-131: Template-generation coverage for context budget is well-scoped.The new dedicated and combined template assertions strengthen regression protection for both budget keys.
132-161: Negative validation tests are appropriate and high-value.Invalid template inputs for both budget keys are now explicitly covered, improving safety against malformed config application.
396-410: Legacy compatibility assertions are a strong addition.This block verifies default restoration behavior in legacy scenarios for both status and template flows as intended.
cli.js (2)
3362-3379: Nice format-preserving rewrite.This keeps the auto-inserted budget keys from stripping a UTF-8 BOM or mixing CRLF/LF in legacy configs.
10095-10119: Good error-path handling here.Gating the fallback with
hasConfigLoadError(...)avoids advertising healthy-looking budget defaults whenconfig.tomlcould not be read or parsed.Also applies to: 11595-11618
|
Re-review PR 🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/unit/provider-share-command.test.mjs (1)
492-497:⚠️ Potential issue | 🟡 MinorStatus-case extraction is still brittle to formatting/quote-style changes.
This extraction still depends on an exact case label string, so harmless formatting edits can break the test harness.
🧪 Non-breaking hardening
- const statusCaseBlock = extractBlockBySignature( - cliSource, - "case 'status': {" - ) + const statusCaseHeaderMatch = cliSource.match(/^\s*case\s+['"]status['"]:\s*\{/m); + assert(statusCaseHeaderMatch, 'status case should exist'); + const statusCaseBlock = extractBlockBySignature( + cliSource, + statusCaseHeaderMatch[0].trim() + ) .replace(/^case\s+['"]status['"]:\s*/, '') - .replace(/\bbreak;\s*(?=\}\s*$)/, ''); + .replace(/\bbreak\s*;\s*(?=\}\s*$)/, '');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/provider-share-command.test.mjs` around lines 492 - 497, The test's extraction of the "status" case is brittle because it matches the literal string "case 'status': {"; update the call that uses extractBlockBySignature and the following .replace chain to match the case label more robustly (e.g., use a regex that allows any quote style and flexible whitespace such as /case\s+['"`]status['"`]\s*:/ or extend extractBlockBySignature to accept a regex signature), then remove the leading case label and trailing break with regex-based replace operations instead of exact string replaces so formatting or quote-style changes won't break tests; ensure you reference the existing extractBlockBySignature invocation and the subsequent .replace calls when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/unit/provider-share-command.test.mjs`:
- Around line 492-497: The test's extraction of the "status" case is brittle
because it matches the literal string "case 'status': {"; update the call that
uses extractBlockBySignature and the following .replace chain to match the case
label more robustly (e.g., use a regex that allows any quote style and flexible
whitespace such as /case\s+['"`]status['"`]\s*:/ or extend
extractBlockBySignature to accept a regex signature), then remove the leading
case label and trailing break with regex-based replace operations instead of
exact string replaces so formatting or quote-style changes won't break tests;
ensure you reference the existing extractBlockBySignature invocation and the
subsequent .replace calls when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ed3ff408-21b9-4bc5-b594-14f994b2a63d
📒 Files selected for processing (1)
tests/unit/provider-share-command.test.mjs
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-31T12:37:50.988Z
Learning: In PR `#59` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-30T04:47:15.339Z
Learning: In PR `#55` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: SurviveM
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-29T02:42:32.761Z
Learning: The repository maintainer SurviveM (GitHub: SurviveM, repo: SakuraByteCore/codexmate) prefers that CodeRabbit review suggestions do not introduce breaking changes. Reviews should focus on correctness, safety, and quality without recommending refactors or changes that would break existing APIs, interfaces, or behavior.
🔇 Additional comments (7)
tests/unit/provider-share-command.test.mjs (7)
149-216: Strong negative-path coverage for template validation side effects.Great addition: invalid budget values now explicitly verify all write/update paths stay untouched, preventing partial-state regressions.
218-373: Default restoration and formatting-preservation tests are solid.These checks materially improve safety for upgraded configs, including BOM/CRLF preservation and invalid-param rejection.
375-460: Status/default synthesis behavior is well-guarded.The split tests for
readPositiveIntegerConfigValueandbuildMcpStatusPayloadcorrectly pin missing-vs-invalid semantics and config-error behavior.
526-528: Nice fix on lexical-scope assertion resilience.Using a whitespace/quote-tolerant regex here is a good stabilization.
530-618: Queued apply replay coverage is excellent.This test now validates refreshed template usage (
template-2) and confirms pending-state cleanup/end-state invariants.
620-787: UI draft-preservation tests are well targeted.Both
loadAllandapplyCodexConfigDirectpaths now protect in-progress sibling budget edits, which is high-value UX regression coverage.
788-841: Direct-apply failure surfacing is correctly pinned.Good verification that backend validation messages are surfaced and no refresh side effects occur on template-generation failure.
2 participants
Summary
Tests
Summary by CodeRabbit
New Features
UI
API
Bug Fixes
Tests