fix(web-ui): validate provider modal input#80
Conversation
📝 WalkthroughWalkthroughAdded OpenClaw auth-profile support and persistence, introduced a sessions-usage API and client/UI flow, centralized provider input normalization/validation and tightened URL handling, changed session-tab rendering/hydration to defer detail loads, and expanded tests across OpenClaw, providers, sessions, and UI behaviors. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/Browser
participant UI as Web UI
participant Nav as Navigation Methods
participant API as Backend API
participant State as App State
User->>UI: Click "Sessions" tab
UI->>Nav: switchMainTab('sessions')
Nav->>State: prepareSessionTabRender()
Nav->>API: api('list-sessions', params...)
API-->>State: sessions batch
State->>UI: render session list
Note over Nav,State: If scheduleAfterFrame available -> defer loadActiveSessionDetail
par Later frame
Nav->>API: api('get-session-detail', sessionId)
API-->>State: active session detail
State->>UI: fill session detail
end
sequenceDiagram
participant User as User
participant Modal as Provider Modal
participant Methods as Provider Methods
participant API as Backend
User->>Modal: Enter provider name + URL
Modal->>Methods: validate fields (trim, strip slash, pattern, reserved, duplicates)
Methods-->>Modal: field errors or normalized draft
alt valid
Modal->>API: add-provider { name, url (normalized) }
API-->>Modal: success
Modal->>Methods: loadAll() refresh providers
else invalid
Modal->>Modal: show errors, disable submit
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 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 docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unit/providers-validation.test.mjs (1)
83-86: Tighten message assertions with explicit count checks.These assertions validate first message content, but not whether unexpected extra messages were emitted.
Small assertion hardening
assert.deepStrictEqual(messages[0], { text: '操作成功', type: 'success' }); + assert.strictEqual(messages.length, 1); @@ assert.deepStrictEqual(messages[0], { text: 'URL 仅支持 http/https', type: 'error' }); + assert.strictEqual(messages.length, 1);Also applies to: 110-113
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/providers-validation.test.mjs` around lines 83 - 86, The test currently only asserts the content of messages[0] (in tests/unit/providers-validation.test.mjs) which allows extra unexpected messages; update the assertions to also assert the exact count of messages (e.g., assert.strictEqual(messages.length, 1)) before checking messages[0]. Apply the same hardening to the second occurrence that checks messages[0] at the other block (lines around the existing deepStrictEqual) so both places validate both length and content for the messages array.
🤖 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 3458-3460: The validation currently calls
isValidHttpUrl(normalizeBaseUrl(url)) but never persists the normalized value,
so later writes still use the raw url; capture the normalized value (e.g., const
normalized = normalizeBaseUrl(url)) before validation and use that normalized
variable in the subsequent write operations instead of the original url (replace
occurrences used at the later write sites with the normalized variable),
ensuring normalizeBaseUrl and isValidHttpUrl calls remain in place.
---
Nitpick comments:
In `@tests/unit/providers-validation.test.mjs`:
- Around line 83-86: The test currently only asserts the content of messages[0]
(in tests/unit/providers-validation.test.mjs) which allows extra unexpected
messages; update the assertions to also assert the exact count of messages
(e.g., assert.strictEqual(messages.length, 1)) before checking messages[0].
Apply the same hardening to the second occurrence that checks messages[0] at the
other block (lines around the existing deepStrictEqual) so both places validate
both length and content for the messages array.
🪄 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: 99699d8f-710b-407f-b3e5-1eac886a1387
📒 Files selected for processing (9)
cli.jstests/e2e/test-config.jstests/unit/provider-switch-regression.test.mjstests/unit/providers-validation.test.mjstests/unit/run.mjstests/unit/web-ui-behavior-parity.test.mjsweb-ui/modules/app.methods.providers.mjsweb-ui/partials/index/modals-basic.htmlweb-ui/styles/modals-core.css
📜 Review details
🔇 Additional comments (22)
web-ui/modules/app.methods.providers.mjs (4)
1-50: Well-structured validation helpers with centralized normalization.The validation layer is cleanly separated with pure functions handling normalization and validation. The
normalizeProviderUrlcorrectly strips trailing slashes, andisValidHttpUrlproperly validates protocol using the URL constructor.
52-91: Edit mode skips name pattern validation intentionally.The validation logic treats edit mode differently - it only checks that
editingNameis non-empty (line 74-76) without validating name pattern or reserved names. This is correct because the name field is readonly in edit mode (per the HTML template), so the name cannot be changed. The validation focus in edit mode is appropriately on the URL field.
139-163:addProvidercorrectly uses normalized values from validation.The flow is correct: normalize draft state first, run validation, and if valid, submit the normalized
nameandurlfrom the validation result rather than the raw draft values. This ensures consistent data is sent to the backend.
280-308:updateProvidervalidation flow is correct.The normalization happens before the try-catch block (line 286), so even if the API call throws, the
editingProviderretains the normalized/trimmed values. This aligns with the test expectation update inprovider-switch-regression.test.mjs.tests/unit/provider-switch-regression.test.mjs (1)
288-311: Test expectation correctly updated for normalization behavior.The test setup (line 63) provides a URL with leading/trailing whitespace:
' https://api.example.com/v1 '. SincenormalizeProviderDraftStateis called before the API request inupdateProvider(line 286 in app.methods.providers.mjs), theeditingProvider.urlis trimmed even when the request throws. The updated expectation at line 301 correctly reflects this behavior.tests/unit/run.mjs (1)
38-38: New test module registered correctly.The new
providers-validation.test.mjstest file is properly added to the test runner, following the established import pattern.tests/e2e/test-config.js (2)
314-326: Comprehensive E2E coverage for URL validation on add-provider.The tests properly verify:
- API rejects invalid URL format (
'not-a-url')- CLI also rejects invalid URL with non-zero exit status
- Failed add-provider does not pollute the provider list
This ensures both API and CLI paths enforce URL validation consistently.
1367-1368: E2E test for update-provider URL validation.Good addition testing that
update-providerrejects non-HTTP/HTTPS URLs (ftp://bad.example.com). This complements the add-provider validation tests and ensures update flows have the same validation.web-ui/partials/index/modals-basic.html (2)
8-36: Add provider modal validation UI is well-implemented.The validation bindings are correct:
- Conditional
invalidclass driven byproviderFieldError('add', field)@blurtriggers normalization to trim whitespace/trailing slashes- Inline error hints displayed when validation fails
- Submit button disabled until
canSubmitProvider('add')returns trueThe
autocomplete="off"andspellcheck="false"attributes improve the input experience for technical fields.
51-68: Edit provider modal validation follows the same pattern.The URL field in the edit modal correctly applies the same validation approach. The name field remains readonly (line 47), which aligns with the backend behavior where provider names are immutable after creation.
web-ui/styles/modals-core.css (2)
252-264: Invalid input state styling is complete and accessible.The CSS correctly handles all interaction states for invalid inputs:
- Border color changes to error color on invalid state
- Maintains error border on hover and focus
- Focus ring uses an error-themed shadow for clear visual feedback
460-464: Error hint styling overrides base opacity correctly.Setting
opacity: 1ensures error messages are fully visible, overriding theopacity: 0.8from the base.form-hintrule.tests/unit/web-ui-behavior-parity.test.mjs (2)
336-342: Refactored drift assertions for consistency.The non-HEAD path now uses the same Set-based filtering pattern as the HEAD path (lines 329-334). This improves code consistency without changing behavior.
350-378: Allowlist updated for new provider validation methods.The 8 new method keys correspond exactly to the methods exported from
createProvidersMethodsinapp.methods.providers.mjs:
canSubmitProvider,findProviderByName,getProviderValidationisReservedProviderCreationName,isValidProviderNameInput,isValidProviderUrlInputnormalizeProviderDraft,providerFieldErrorThis ensures the parity test doesn't fail due to intentional API additions.
cli.js (3)
3539-3541: Good guard for update API URL validation.Line 3539 rejects non-HTTP(S) URLs early in backend update flow, matching the new validation contract.
8237-8237: CLI add flow normalization/validation is correctly enforced.Normalizing at Line 8237 and validating at Line 8259 prevents malformed schemes and keeps stored base URLs consistent.
Also applies to: 8259-8262
8320-8320: CLI update flow correctly validates and writes normalized URLs.Line 8320 normalization + Line 8349 validation + Line 8478 conditional replacement is a solid, consistent update path.
Also applies to: 8349-8352, 8478-8480
tests/unit/providers-validation.test.mjs (5)
29-54: Great pre-submit guard coverage for invalid add flow.This test correctly verifies field-level errors, disabled submit state, API suppression, and surfaced error messaging in one path.
56-87: Normalization payload test is solid.Good assertion set for trimming/URL normalization, modal reset state, reload trigger, and success feedback.
89-114: Edit-path invalid URL blocking is well covered.This captures the expected guardrail behavior:
canSubmitProvider('edit')false, no API call, and user-facing error.
116-127: Reserved-name regression test is clear and targeted.Good direct check for reserved-name error and submit lockout.
1-3: No action required —testis available via global injection in the test bootstrap.The file does not import
test, but that is correct. The test runner bootstrap (tests/unit/run.mjs) explicitly injectsglobalThis.testbefore execution, so thetest(...)calls throughout this file will resolve correctly. No import is needed.
|
@coderabbitai review |
✅ Actions performedReview 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)
cli.js (1)
8238-8247:⚠️ Potential issue | 🟡 MinorDifferentiate blank vs invalid URLs in
cmdAdd.Line 8238 normalizes before the required-field check, so inputs like
/or////collapse to''and hit"名称和URL必填"instead of the new"URL 仅支持 http/https"validation. That makes the CLI add flow less precise than the API/web and update flows.💡 Suggested fix
function cmdAdd(name, baseUrl, apiKey, silent = false) { const providerName = typeof name === 'string' ? name.trim() : ''; - const providerBaseUrl = normalizeBaseUrl(baseUrl); + const rawBaseUrl = typeof baseUrl === 'string' ? baseUrl.trim() : ''; + const providerBaseUrl = normalizeBaseUrl(rawBaseUrl); - if (!providerName || !providerBaseUrl) { + if (!providerName || !rawBaseUrl) { if (!silent) { console.error('用法: codexmate add <名称> <URL> [密钥]'); console.log('\n示例:'); console.log(' codexmate add 88code https://api.88code.ai/v1 sk-xxx'); } throw new Error('名称和URL必填'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 8238 - 8247, Move the required-field check to use the raw baseUrl before calling normalizeBaseUrl so blank inputs like "/" or "////" are caught as a missing URL; specifically, in the cmdAdd flow check (!providerName || !baseUrl) first and throw the existing "名称和URL必填" message, then call normalizeBaseUrl(baseUrl) to get providerBaseUrl and perform the URL validation that yields "URL 仅支持 http/https" when normalizeBaseUrl returns an invalid URL; update the branch that currently uses providerBaseUrl so it validates normalized URL separately from the presence check (references: normalizeBaseUrl and providerBaseUrl).
🧹 Nitpick comments (3)
tests/unit/reset-main.test.mjs (1)
118-121: Make this assertion less formatting-sensitive.Line 120 hard-codes an exact source snippet, so minor formatting changes can fail the test even when behavior is unchanged. Consider a whitespace-tolerant regex (or behavior-level mocking test) to reduce brittleness.
Suggested lightweight tweak
- assert.match(content, /run\('git rev-parse --is-inside-work-tree', \{ stdio: 'ignore' \}\);/); + assert.match( + content, + /run\(\s*'git rev-parse --is-inside-work-tree'\s*,\s*\{\s*stdio:\s*'ignore'\s*\}\s*\);/ + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/reset-main.test.mjs` around lines 118 - 121, The test "git repository probe stays silent during reset startup" currently asserts an exact source snippet which is brittle; update the assertion in tests/unit/reset-main.test.mjs to use a whitespace-tolerant regex (or replace the file-content assertion with a behavior-level mock) that matches the command and the stdio option irrespective of spacing/quotes—e.g., build a regex that looks for run\(\s*['"`]git rev-parse --is-inside-work-tree['"`]\s*,\s*\{\s*stdio\s*:\s*['"`]ignore['"`]\s*\}\s*\) and use that with assert.match on the content variable (or alternatively mock the run invocation and assert it was called with the expected args) so formatting changes won’t break the test.cli.js (1)
3452-3461: Consider one shared provider URL validation helper.The same
normalizeBaseUrl(...)+isValidHttpUrl(...)rule is now spread across add/update helper paths and CLI paths. Extracting a small shared validator, called after each flow’s own presence checks, would make the Web UI/API and CLI behavior much harder to drift out of sync again.Also applies to: 3540-3542, 8238-8263, 8321-8353
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 3452 - 3461, Extract a shared validator function (e.g., validateProviderUrl) that encapsulates normalizeBaseUrl + isValidHttpUrl logic and error messages, then replace the duplicated checks in CLI and helper flows by calling validateProviderUrl(url) immediately after presence checks (after verifying name and url exist) in functions that currently call normalizeBaseUrl and isValidHttpUrl; update call sites including the block using normalizeBaseUrl/isValidHttpUrl, and the other locations mentioned so the CLI, add/update helpers and Web/API use the same validateProviderUrl function to keep behavior consistent.tests/unit/config-tabs-ui.test.mjs (1)
296-296: Consider de-brittling the__mainTabSwitchStateshape assertion.Line 296 hard-codes field order in one regex. Semantically equivalent reorder/formatting can fail the test unnecessarily. Prefer separate
assert.matchchecks per field to keep intent stable.🤖 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` at line 296, The single regex asserting the entire __mainTabSwitchState object shape is brittle due to field order/formatting; update the test to validate each property independently by replacing the one assert.match(appScript, /__mainTabSwitchState: .../) with multiple assert.match calls against appScript that each check a single field (e.g., /__mainTabSwitchState[\s\S]*intent:\s*''/, /pendingTarget:\s*''/, /pendingConfigMode:\s*''/, /ticket:\s*0/) or alternatively use a single regex with unordered lookaheads for each field; target the __mainTabSwitchState symbol and the appScript variable 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.
Inline comments:
In `@web-ui/app.js`:
- Around line 119-124: The data property __mainTabSwitchState is using leading
underscores so it won't be proxied to the component instance; rename the data
key to a non-reserved name (for example mainTabSwitchState or uiTabSwitchState)
in the component's data() and update all places that reference
this.__mainTabSwitchState (notably in app.methods.navigation.mjs and any
navigation methods) to use the new property name (this.mainTabSwitchState or
this.uiTabSwitchState) so the component can access and mutate the state at
runtime.
In `@web-ui/partials/index/layout-header.html`:
- Around line 297-301: The template currently only guards for card existence
(sessionUsageSummaryCards[0] and [1]) so if the card exists but its value is
null/undefined the UI stops falling back to 0; update both bindings to use the
card value with a nullish fallback, e.g. use optional chaining with a nullish
coalescing fallback (sessionUsageSummaryCards[0]?.value ?? 0 and
sessionUsageSummaryCards[1]?.value ?? 0) so missing values render as 0 while
preserving existing behavior when the card itself is absent.
---
Outside diff comments:
In `@cli.js`:
- Around line 8238-8247: Move the required-field check to use the raw baseUrl
before calling normalizeBaseUrl so blank inputs like "/" or "////" are caught as
a missing URL; specifically, in the cmdAdd flow check (!providerName ||
!baseUrl) first and throw the existing "名称和URL必填" message, then call
normalizeBaseUrl(baseUrl) to get providerBaseUrl and perform the URL validation
that yields "URL 仅支持 http/https" when normalizeBaseUrl returns an invalid URL;
update the branch that currently uses providerBaseUrl so it validates normalized
URL separately from the presence check (references: normalizeBaseUrl and
providerBaseUrl).
---
Nitpick comments:
In `@cli.js`:
- Around line 3452-3461: Extract a shared validator function (e.g.,
validateProviderUrl) that encapsulates normalizeBaseUrl + isValidHttpUrl logic
and error messages, then replace the duplicated checks in CLI and helper flows
by calling validateProviderUrl(url) immediately after presence checks (after
verifying name and url exist) in functions that currently call normalizeBaseUrl
and isValidHttpUrl; update call sites including the block using
normalizeBaseUrl/isValidHttpUrl, and the other locations mentioned so the CLI,
add/update helpers and Web/API use the same validateProviderUrl function to keep
behavior consistent.
In `@tests/unit/config-tabs-ui.test.mjs`:
- Line 296: The single regex asserting the entire __mainTabSwitchState object
shape is brittle due to field order/formatting; update the test to validate each
property independently by replacing the one assert.match(appScript,
/__mainTabSwitchState: .../) with multiple assert.match calls against appScript
that each check a single field (e.g., /__mainTabSwitchState[\s\S]*intent:\s*''/,
/pendingTarget:\s*''/, /pendingConfigMode:\s*''/, /ticket:\s*0/) or
alternatively use a single regex with unordered lookaheads for each field;
target the __mainTabSwitchState symbol and the appScript variable when making
the change.
In `@tests/unit/reset-main.test.mjs`:
- Around line 118-121: The test "git repository probe stays silent during reset
startup" currently asserts an exact source snippet which is brittle; update the
assertion in tests/unit/reset-main.test.mjs to use a whitespace-tolerant regex
(or replace the file-content assertion with a behavior-level mock) that matches
the command and the stdio option irrespective of spacing/quotes—e.g., build a
regex that looks for run\(\s*['"`]git rev-parse
--is-inside-work-tree['"`]\s*,\s*\{\s*stdio\s*:\s*['"`]ignore['"`]\s*\}\s*\) and
use that with assert.match on the content variable (or alternatively mock the
run invocation and assert it was called with the expected args) so formatting
changes won’t break the test.
🪄 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: 0e40ab66-06b3-414d-ad65-0fe4bad8a946
📒 Files selected for processing (19)
README.en.mdREADME.mdcli.jscmd/reset-main.jsres/vue.global.prod.jstests/e2e/test-config.jstests/unit/config-tabs-ui.test.mjstests/unit/providers-validation.test.mjstests/unit/reset-main.test.mjstests/unit/session-tab-switch-performance.test.mjstests/unit/web-ui-behavior-parity.test.mjsweb-ui/app.jsweb-ui/index.htmlweb-ui/modules/app.methods.session-browser.mjsweb-ui/partials/index/layout-header.htmlweb-ui/partials/index/panel-docs.htmlweb-ui/partials/index/panel-usage.htmlweb-ui/session-helpers.mjsweb-ui/styles/docs-panel.css
✅ Files skipped from review due to trivial changes (5)
- web-ui/index.html
- README.en.md
- README.md
- web-ui/partials/index/panel-usage.html
- tests/unit/web-ui-behavior-parity.test.mjs
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/e2e/test-config.js
- tests/unit/providers-validation.test.mjs
📜 Review details
🔇 Additional comments (8)
cmd/reset-main.js (1)
163-163: Good hardening for startup probe.Line 163 correctly silences the
git rev-parse --is-inside-work-treecheck without changing failure behavior. Nice small UX improvement.web-ui/modules/app.methods.session-browser.mjs (1)
362-366: LGTM!The signature update and options forwarding correctly align with the updated
loadSessionsHelperAPI.Minor nit:
options || {}is redundant given the default parameteroptions = {}, but it's harmless defensive coding.tests/unit/session-tab-switch-performance.test.mjs (2)
88-119: LGTM!Test correctly validates that:
- Switching to the usage tab calls
loadSessionswith{ includeActiveDetail: false }- Repeated switches don't trigger additional loads (checking array length stays at 1)
462-511: LGTM!Comprehensive test coverage for the
includeActiveDetail: falsepath:
- Verifies
loadActiveSessionDetailis not called- Confirms
activeSessionis still updated from returned sessions- Validates stale
activeSessionMessagesare clearedweb-ui/session-helpers.mjs (2)
14-50: LGTM!The
switchMainTabchanges correctly computeshouldLoadSessionDetailbased on the destination tab and pass it through toloadSessions. This optimizes the usage tab path by skipping unnecessary detail fetches.
93-152: DefaultincludeActiveDetailbehavior is safe across all callers.The optimization correctly skips detail loading when users are not on the sessions tab. All callers are covered:
session-actions.mjs(line 393): Safe —selectSession(matched)always callsloadActiveSessionDetail()unconditionally.session-browser.mjsfilter handlers (lines 289, 294, 298): Safe — these handlers execute only when the sessions component is active, sothis.mainTab === 'sessions'evaluates to true.session-trash.mjs(line 346): Detail loading is deferred, not skipped. When users interact with the recovered session,selectSession()loads detail on demand.This is intentional lazy loading aligned with the optimization goal.
web-ui/partials/index/panel-docs.html (1)
97-100: Command-row markup update looks good.This aligns cleanly with the new flex row layout and keeps the copy action safely disabled when
target.commandis empty.web-ui/styles/docs-panel.css (1)
88-97: Responsive command-row styling is solid.The new
.docs-command-rowlayout and mobile stacking rules are coherent and match the updated docs panel markup.Also applies to: 174-176
| __mainTabSwitchState: { | ||
| intent: '', | ||
| pendingTarget: '', | ||
| pendingConfigMode: '', | ||
| ticket: 0 | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Vue 3 (Options API), are data() properties whose names start with "_" or "$" proxied onto this? What is the recommended pattern for internal reactive state names to ensure reactivity and template/method access?
💡 Result:
No. In Vue 3’s Options API, data() keys that start with _ or $ are made reactive, but they are not proxied onto the component instance, so they won’t be available as this._foo / this.$foo (and likewise won’t be directly available in template expressions). You must access them via this.$data._foo (or $data._foo in templates). [1]
Recommended pattern for internal reactive state (Options API)
- Don’t use
_or$prefixes fordata()top-level keys; those prefixes are reserved for Vue’s own instance APIs / internals. [1][2] - If you want “internal-only” state, use a non-reserved naming convention (e.g.
internalFoo,fooInternal,uiFoo,debugFoo), or group internal state under a single object likestate/ui/form:This keeps full reactivity and allows direct access in both methods (export default { data() { return { ui: { isSaving: false }, internalFoo: 123 } }, methods: { save() { this.ui.isSaving = true } } }
this.ui.isSaving) and templates ({{ ui.isSaving }}), while avoiding reserved prefixes. [1][2]
Sources:
[1] Vue 3 API Reference — Options: State (data, reserved _/$ prefixes and $data access) (vuejs.org)
[2] Vue Guide — Reactivity Fundamentals (Options API: data() is proxied onto this, and $ / _ prefixes are reserved/should be avoided) (vuejs.org)
Citations:
- 1: https://vuejs.org/api/options-state
- 2: https://vuejs.org/guide/essentials/reactivity-fundamentals.html?noteId=note-cc0b26eb-a6c6-4372-91bf-1725f76529cd
🏁 Script executed:
# Search for all references to __mainTabSwitchState in the web-ui directory
rg "__mainTabSwitchState" web-ui/Repository: SakuraByteCore/codexmate
Length of output: 472
Rename __mainTabSwitchState — underscore-prefixed data keys are not proxied to this in Vue 3.
In Vue 3's Options API, data() properties starting with _ or $ are made reactive but are not proxied onto the component instance. The code at lines 119–124 defines __mainTabSwitchState, but the navigation methods in app.methods.navigation.mjs access this.__mainTabSwitchState, which will be undefined at runtime.
Change __mainTabSwitchState to mainTabSwitchState (remove the leading underscores), or rename it to follow a non-reserved convention like tabSwitchState, uiTabSwitchState, etc.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-ui/app.js` around lines 119 - 124, The data property __mainTabSwitchState
is using leading underscores so it won't be proxied to the component instance;
rename the data key to a non-reserved name (for example mainTabSwitchState or
uiTabSwitchState) in the component's data() and update all places that reference
this.__mainTabSwitchState (notably in app.methods.navigation.mjs and any
navigation methods) to use the new property name (this.mainTabSwitchState or
this.uiTabSwitchState) so the component can access and mutate the state at
runtime.
| <span class="value">{{ sessionUsageSummaryCards[0] ? sessionUsageSummaryCards[0].value : 0 }}</span> | ||
| </div> | ||
| <div class="status-chip"> | ||
| <span class="label">总消息数</span> | ||
| <span class="value">{{ sessionUsageSummaryCards[1]?.value ?? 0 }}</span> | ||
| <span class="value">{{ sessionUsageSummaryCards[1] ? sessionUsageSummaryCards[1].value : 0 }}</span> |
There was a problem hiding this comment.
Preserve 0 fallback when summary card exists but value is missing.
Line 297 and Line 301 now only guard card existence. If value is null/undefined, the UI no longer falls back to 0.
💡 Proposed fix
-<span class="value">{{ sessionUsageSummaryCards[0] ? sessionUsageSummaryCards[0].value : 0 }}</span>
+<span class="value">{{ (sessionUsageSummaryCards[0] && sessionUsageSummaryCards[0].value != null) ? sessionUsageSummaryCards[0].value : 0 }}</span>
-<span class="value">{{ sessionUsageSummaryCards[1] ? sessionUsageSummaryCards[1].value : 0 }}</span>
+<span class="value">{{ (sessionUsageSummaryCards[1] && sessionUsageSummaryCards[1].value != null) ? sessionUsageSummaryCards[1].value : 0 }}</span>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-ui/partials/index/layout-header.html` around lines 297 - 301, The
template currently only guards for card existence (sessionUsageSummaryCards[0]
and [1]) so if the card exists but its value is null/undefined the UI stops
falling back to 0; update both bindings to use the card value with a nullish
fallback, e.g. use optional chaining with a nullish coalescing fallback
(sessionUsageSummaryCards[0]?.value ?? 0 and sessionUsageSummaryCards[1]?.value
?? 0) so missing values render as 0 while preserving existing behavior when the
card itself is absent.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/unit/web-ui-behavior-parity.test.mjs (1)
380-386: Minor inconsistency: Missing error message in assertions.The non-HEAD method key assertions at lines 384-385 lack the error message parameter that is present in the data key assertions (lines 345-346) and the HEAD branch method assertions (lines 377-378). This makes debugging test failures harder.
🔧 Suggested fix for consistency
const unexpectedExtraCurrentMethodKeys = extraCurrentMethodKeys.filter((key) => !allowedExtraMethodKeySet.has(key)); const unexpectedMissingCurrentMethodKeys = missingCurrentMethodKeys.filter((key) => !allowedMissingMethodKeySet.has(key)); - assert.deepStrictEqual(unexpectedExtraCurrentMethodKeys, []); - assert.deepStrictEqual(unexpectedMissingCurrentMethodKeys, []); + assert.deepStrictEqual(unexpectedExtraCurrentMethodKeys, [], `unexpected extra method keys against ${parityBaseline.ref}`); + assert.deepStrictEqual(unexpectedMissingCurrentMethodKeys, [], `unexpected missing method keys against ${parityBaseline.ref}`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/web-ui-behavior-parity.test.mjs` around lines 380 - 386, The two assertions comparing unexpectedExtraCurrentMethodKeys and unexpectedMissingCurrentMethodKeys lack descriptive error messages; update the assert.deepStrictEqual calls for unexpectedExtraCurrentMethodKeys and unexpectedMissingCurrentMethodKeys to include the same style of failure messages used elsewhere (e.g., referencing allowedExtraCurrentMethodKeys/allowedMissingCurrentMethodKeys and currentMethodKeys) so test failures are easier to debug—locate the assertions using the variable names unexpectedExtraCurrentMethodKeys and unexpectedMissingCurrentMethodKeys and add appropriate string messages as the third argument to assert.deepStrictEqual.
🤖 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 5454-5465: When mapping the result of listAllSessions(...) in the
anonymous mapping function, preserve and apply the exact count before dropping
__messageCountExact: if normalized.__messageCountExact != null then set
normalized.messageCount = normalized.__messageCountExact (or merge exact value
into whatever messageCount field is used) and only then delete
normalized.__messageCountExact; do this inside the same mapping callback so
listAllSessions and the mapping produce hydrated sessions with exact counts.
- Around line 10732-10744: The case 'list-sessions-usage' block fails if params
is undefined and also fails to pass the normalized source to listSessionUsage;
guard params first (e.g., ensure params is an object before accessing
params.source), compute a normalized source string from params.source
(trim().toLowerCase()), validate it (allow 'codex','claude','all'), and then
pass that normalized source into listSessionUsage (instead of the raw
params.source) while preserving other params; update the result assignment in
the 'list-sessions-usage' handler to use the normalized source and avoid reading
properties when params is missing.
---
Nitpick comments:
In `@tests/unit/web-ui-behavior-parity.test.mjs`:
- Around line 380-386: The two assertions comparing
unexpectedExtraCurrentMethodKeys and unexpectedMissingCurrentMethodKeys lack
descriptive error messages; update the assert.deepStrictEqual calls for
unexpectedExtraCurrentMethodKeys and unexpectedMissingCurrentMethodKeys to
include the same style of failure messages used elsewhere (e.g., referencing
allowedExtraCurrentMethodKeys/allowedMissingCurrentMethodKeys and
currentMethodKeys) so test failures are easier to debug—locate the assertions
using the variable names unexpectedExtraCurrentMethodKeys and
unexpectedMissingCurrentMethodKeys and add appropriate string messages as the
third argument to assert.deepStrictEqual.
🪄 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: 98655830-8861-4780-9b33-f14214c7345c
📒 Files selected for processing (16)
cli.jstests/e2e/test-sessions.jstests/unit/config-tabs-ui.test.mjstests/unit/openclaw-persist-regression.test.mjstests/unit/session-tab-switch-performance.test.mjstests/unit/web-ui-behavior-parity.test.mjsweb-ui/app.jsweb-ui/modules/app.computed.session.mjsweb-ui/modules/app.methods.openclaw-persist.mjsweb-ui/modules/app.methods.session-actions.mjsweb-ui/modules/app.methods.session-browser.mjsweb-ui/modules/app.methods.session-trash.mjsweb-ui/partials/index/modal-openclaw-config.htmlweb-ui/partials/index/panel-config-openclaw.htmlweb-ui/partials/index/panel-usage.htmlweb-ui/session-helpers.mjs
✅ Files skipped from review due to trivial changes (2)
- web-ui/partials/index/panel-config-openclaw.html
- tests/unit/config-tabs-ui.test.mjs
🚧 Files skipped from review as they are similar to previous changes (4)
- web-ui/partials/index/panel-usage.html
- web-ui/modules/app.methods.session-browser.mjs
- tests/unit/session-tab-switch-performance.test.mjs
- 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-04-04T01:36:26.964Z
Learning: In PR `#68` for SakuraByteCore/codexmate, github-actions[bot] again flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint (reinforced across PRs `#55`, `#59`, `#67`, `#68`) is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-31T12:37:51.023Z
Learning: In PR `#59` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-04-03T10:42:22.368Z
Learning: In PR `#67` for SakuraByteCore/codexmate, github-actions[bot] flagged that CodeRabbit was again introducing breaking changes. The maintainer's absolute constraint (reinforced across PRs `#55`, `#59`, `#67`) is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-30T04:47:15.352Z
Learning: In PR `#55` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: SurviveM
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-29T02:42:32.785Z
Learning: The repository maintainer SurviveM (GitHub: SurviveM, repo: SakuraByteCore/codexmate) prefers that CodeRabbit review suggestions do not introduce breaking changes. Reviews should focus on correctness, safety, and quality without recommending refactors or changes that would break existing APIs, interfaces, or behavior.
🔇 Additional comments (24)
tests/unit/web-ui-behavior-parity.test.mjs (1)
322-367: Allowlist expansions align with new features.The data key allowlist additions (
sessionsUsage*,__mainTabSwitchState) and method key allowlist additions (provider validation helpers likecanSubmitProvider,isValidProviderUrlInput, etc., plussyncDefaultOpenclawConfigEntry) correctly track the expected drift from the new provider modal validation and sessions-usage features.web-ui/partials/index/modal-openclaw-config.html (1)
261-268: UI disabled state aligns with backend guard.The conditional warning text and the additional disabled condition for the save button correctly reflect the backend guard in
saveOpenclawConfig()(context snippet 1 at lines 253-256) that prevents direct saving of the default config. This provides clear user feedback when editing '默认配置'.tests/unit/openclaw-persist-regression.test.mjs (4)
61-63: Context stub update aligns with implementation.Adding
saveOpenclawConfigs()returningtrueto the base context ensures tests can verify the persistence flow. This matches the method signature fromapp.methods.openclaw-editing.mjs(context snippet 2).
124-156: Good coverage for default config refresh behavior.This test verifies that editing '默认配置' always fetches fresh content from the API, updates the persisted entry, and preserves
currentOpenclawConfig. The assertion at line 155 correctly ensures the current selection is unchanged.
269-286: Correctly tests the save guard for default config.This test verifies that
saveOpenclawConfig()refuses to save when editing the default system config, showing the expected info message and not settingopenclawSavingto true.
398-427: Good coverage for default entry synchronization after apply.This test ensures
saveAndApplyOpenclawConfig()updates the '默认配置' entry content to match the applied draft, and that path/existence are correctly set from the API response.web-ui/modules/app.methods.openclaw-persist.mjs (8)
24-45: Default config state sync helper is well-structured.The
syncDefaultOpenclawConfigStatefunction correctly:
- Normalizes the configs object to ensure the default entry exists
- Updates path/exists/lineEnding when provided
- Resets
currentOpenclawConfigto default when the current selection is invalid- Conditionally persists via
saveOpenclawConfigs()Note: Lines 26-29 rebuild configs then immediately overwrite the default entry, which is slightly redundant but ensures the content is always the latest
nextContent.
54-77: New sync method correctly fetches and updates default config.
syncDefaultOpenclawConfigEntryproperly:
- Fetches the config file via API
- Falls back to
defaultOpenclawTemplatewhen content is empty- Handles errors gracefully with optional silent mode
- Returns the response for chaining
The implementation follows the existing pattern of other methods that call
api()directly.
101-122: Default config edit correctly forces refresh from file.When editing '默认配置', setting
force: true,fallbackToTemplate: true, andsyncDefaultEntry: trueensures the modal always shows the current system config state rather than stale cached content.
142-143: Default entry sync during load is correctly implemented.The
syncDefaultEntryflag auto-detects when editing the default config (line 142-143) and triggerssyncDefaultOpenclawConfigStateafter loading (lines 183-190). This ensures the persisted default entry reflects the actual file content.Also applies to: 183-190
253-256: Guard prevents unintended local-only save of default config.The guard correctly blocks
saveOpenclawConfig()when editing '默认配置' and provides clear user feedback to use "保存并应用" instead. This aligns with the UI disabled state in the modal template.
286-291: Default entry stays synchronized after apply.After a successful
saveAndApplyOpenclawConfig, callingsyncDefaultOpenclawConfigStatewith the applied content ensures '默认配置' always reflects the current system configuration.
302-305: Deletion guard protects the default config entry.Preventing deletion of '默认配置' ensures there's always a default entry representing the system config. The informative message explains why deletion isn't allowed.
341-346: Consistent sync in applyOpenclawConfig.The sync after successful apply in
applyOpenclawConfigmirrors the behavior insaveAndApplyOpenclawConfig, ensuring the default entry is updated regardless of which apply path is used.web-ui/modules/app.computed.session.mjs (1)
105-107: Good data-source switch for usage charts.Using
sessionsUsageListhere correctly binds chart aggregation to the dedicated usage API dataset and keeps the summary cards consistent with usage-specific loading/invalidation flow.cli.js (1)
3452-3460: Nice fix: the normalized URL is now what gets stored.Validation and persisted
base_urlnow use the same normalized value, so trailing-slash input no longer round-trips back into config.Also applies to: 3503-3503
web-ui/modules/app.methods.session-trash.mjs (1)
343-345: LGTM!The conditional invalidation of session usage data after a successful restore is correctly guarded with a
typeofcheck, ensuring backward compatibility. The placement after the success message and before the trash list refresh is appropriate.web-ui/modules/app.methods.session-actions.mjs (2)
392-394: LGTM!The conditional invalidation after cloning a session is correctly guarded and placed appropriately after the success message.
436-438: LGTM!The conditional invalidation after trashing a session follows the same safe pattern used in
cloneSessionandrestoreSessionTrash, maintaining consistency across the codebase.tests/e2e/test-sessions.js (1)
37-45: LGTM!The new E2E tests for
list-sessions-usageare well-structured:
- Validates the happy path with
source: 'all'- Correctly asserts presence of both Codex and Claude entries
- Properly verifies that internal hydration markers (
__messageCountExact) are not exposed in the API response- Includes negative test for invalid source, following the established pattern
web-ui/session-helpers.mjs (4)
18-19: LGTM!Clean separation of tab-entry conditions improves readability and supports the new usage tab behavior.
45-50: LGTM!The tab-switch logic correctly distinguishes between sessions and usage tabs:
- Sessions tab triggers
loadSessionswith explicitincludeActiveDetail: true- Usage tab safely guards the
loadSessionsUsagecall with both atypeofcheck and!sessionsUsageLoadedOnce
96-103: LGTM!The signature change
loadSessions(api, options = {})is backward compatible:
- Default
options = {}ensures existing callers (which pass no arguments) continue to work- The
includeActiveDetailfallback logic preserves the original behavior when the option is not explicitly set- Using
hasOwnPropertycheck correctly distinguishes between "not provided" vs "explicitly set to false"
153-155: LGTM!The conditional
loadActiveSessionDetailcall correctly implements the optimization—detail is only fetched whenincludeActiveDetailis true, avoiding unnecessary API calls when navigating to the usage tab.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
web-ui/modules/app.methods.openclaw-core.mjs (1)
178-178: Unused variableproviderConfig.This variable is assigned but never referenced after line 178. Since provider field extraction now uses
readFirstProviderString(providerRecords, ...)directly, this single-record lookup appears to be leftover from the previous implementation.🧹 Suggested cleanup
const providerRecords = providerName ? [ modelProviders && modelProviders[providerName], rootProviders && rootProviders[providerName] ] : []; - const providerConfig = providerRecords.find((item) => isPlainRecord(item)) || null; const providerModels = readPreferredProviderModels(providerRecords);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/modules/app.methods.openclaw-core.mjs` at line 178, The local variable providerConfig is unused and should be removed to clean up dead code: delete the assignment line that defines providerConfig (const providerConfig = providerRecords.find((item) => isPlainRecord(item)) || null;) and rely on the existing readFirstProviderString(providerRecords, ...) usage to extract the provider; ensure no other references to providerConfig remain and run tests/lint to confirm no regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@web-ui/modules/app.methods.openclaw-core.mjs`:
- Line 178: The local variable providerConfig is unused and should be removed to
clean up dead code: delete the assignment line that defines providerConfig
(const providerConfig = providerRecords.find((item) => isPlainRecord(item)) ||
null;) and rely on the existing readFirstProviderString(providerRecords, ...)
usage to extract the provider; ensure no other references to providerConfig
remain and run tests/lint to confirm no regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cab25233-ef6a-4ff9-98be-a63f4ec52df3
📒 Files selected for processing (2)
tests/unit/openclaw-core.test.mjsweb-ui/modules/app.methods.openclaw-core.mjs
✅ Files skipped from review due to trivial changes (1)
- tests/unit/openclaw-core.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-04-04T01:36:26.964Z
Learning: In PR `#68` for SakuraByteCore/codexmate, github-actions[bot] again flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint (reinforced across PRs `#55`, `#59`, `#67`, `#68`) is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-04-03T10:42:22.368Z
Learning: In PR `#67` for SakuraByteCore/codexmate, github-actions[bot] flagged that CodeRabbit was again introducing breaking changes. The maintainer's absolute constraint (reinforced across PRs `#55`, `#59`, `#67`) is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-31T12:37:51.023Z
Learning: In PR `#59` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-30T04:47:15.352Z
Learning: In PR `#55` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
🔇 Additional comments (4)
web-ui/modules/app.methods.openclaw-core.mjs (4)
1-3: LGTM!Clean helper function that correctly identifies plain objects by checking for truthiness, object type, and non-array status.
5-24: LGTM!These helper functions correctly implement ordered fallback patterns for reading provider fields and models from multiple record sources with legacy key aliases.
181-196: LGTM!The model entry lookup correctly supports both
item.idanditem.modelfield names for backward compatibility with legacy shapes, and properly handles the single-model fallback scenario.
198-217: LGTM!The field extraction with ordered key aliases provides comprehensive backward compatibility for legacy provider configurations. The fallback chains for
modelName,contextWindow, andmaxTokenscorrectly handle both camelCase and snake_case field conventions with proper type validation.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web-ui/modules/app.methods.openclaw-editing.mjs (1)
168-179:⚠️ Potential issue | 🟠 MajorValidate against existing root/alias URL fields too.
fillOpenclawQuickFromConfig()now reads provider URLs from bothconfig.models.providersandconfig.providers, and it acceptsbaseUrl,base_url, andurl. This guard still only checksprovider.baseUrl, so a provider defined only at the root level—or only via an alias field—now fails with请填写 URLas soon asbaseUrlReadOnlyblanks the structured value.🩹 Minimal fix
const models = ensureObject(config.models); const providers = ensureObject(models.providers); const provider = ensureObject(providers[providerName]); + const rootProviders = ensureObject(config.providers); + const inheritedProvider = ensureObject(rootProviders[providerName]); const baseUrl = this.openclawQuick.baseUrlReadOnly ? '' : (this.openclawQuick.baseUrl || '').trim(); - if (!baseUrl && !provider.baseUrl) { + const existingBaseUrl = provider.baseUrl || provider.base_url || provider.url + || inheritedProvider.baseUrl || inheritedProvider.base_url || inheritedProvider.url; + if (!baseUrl && !existingBaseUrl) { this.showMessage('请填写 URL', 'error'); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/modules/app.methods.openclaw-editing.mjs` around lines 168 - 179, In fillOpenclawQuickFromConfig(), the URL presence check only inspects provider.baseUrl but the config can define providers under config.models.providers or config.providers and may use baseUrl, base_url, or url; update the guard that currently reads baseUrl and provider.baseUrl to also look up a root-level provider (from config.providers[providerName]) and to accept provider.base_url and provider.url as valid URL fields (in addition to provider.baseUrl), and only call this.showMessage('请填写 URL', 'error') / return if none of baseUrl, provider.baseUrl, provider.base_url, provider.url, or the corresponding root-level provider URL variants are present.
🤖 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/modules/app.methods.openclaw-core.mjs`:
- Around line 235-236: The code is mistakenly treating the auth-strategy label
"preferred_auth_method" as an API key; update the call that populates
apiKeyField (readFirstProviderDisplayValue(providerRecords, [...])) to remove
"preferred_auth_method" from the candidate keys and ensure credential keys like
"token", "auth_token", "authToken", "key", "token" remain present and
prioritized so the API key box loads actual credentials instead of the auth
strategy label.
---
Outside diff comments:
In `@web-ui/modules/app.methods.openclaw-editing.mjs`:
- Around line 168-179: In fillOpenclawQuickFromConfig(), the URL presence check
only inspects provider.baseUrl but the config can define providers under
config.models.providers or config.providers and may use baseUrl, base_url, or
url; update the guard that currently reads baseUrl and provider.baseUrl to also
look up a root-level provider (from config.providers[providerName]) and to
accept provider.base_url and provider.url as valid URL fields (in addition to
provider.baseUrl), and only call this.showMessage('请填写 URL', 'error') / return
if none of baseUrl, provider.baseUrl, provider.base_url, provider.url, or the
corresponding root-level provider URL variants are present.
🪄 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: 4b3b2d27-6109-4b13-91a2-49a6f2bd6331
📒 Files selected for processing (7)
tests/unit/openclaw-core.test.mjstests/unit/openclaw-editing.test.mjstests/unit/run.mjsweb-ui/app.jsweb-ui/modules/app.methods.openclaw-core.mjsweb-ui/modules/app.methods.openclaw-editing.mjsweb-ui/partials/index/modal-openclaw-config.html
✅ Files skipped from review due to trivial changes (1)
- tests/unit/run.mjs
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/unit/openclaw-core.test.mjs
- web-ui/partials/index/modal-openclaw-config.html
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-04-04T01:36:26.964Z
Learning: In PR `#68` for SakuraByteCore/codexmate, github-actions[bot] again flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint (reinforced across PRs `#55`, `#59`, `#67`, `#68`) is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-04-03T10:42:22.368Z
Learning: In PR `#67` for SakuraByteCore/codexmate, github-actions[bot] flagged that CodeRabbit was again introducing breaking changes. The maintainer's absolute constraint (reinforced across PRs `#55`, `#59`, `#67`) is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-31T12:37:51.023Z
Learning: In PR `#59` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-30T04:47:15.352Z
Learning: In PR `#55` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
🔇 Additional comments (3)
web-ui/app.js (2)
119-124: Avoid_-prefixeddata()keys here.In Vue 3's Options API,
data()keys starting with_or$stay reactive but are not proxied ontothis. If any method or template readsthis.__mainTabSwitchState/__mainTabSwitchState, it will resolve asundefined.#!/bin/bash set -euo pipefail rg -n -C2 '\b(this\.__mainTabSwitchState|\$data\.__mainTabSwitchState|__mainTabSwitchState\b)' web-uiIf the matches show
this.__mainTabSwitchStateor bare template access instead of$data.__mainTabSwitchState, rename the key or switch those accesses to$data.
404-405: I need the review comment to rewrite. Please provide the comment within<review_comment>tags along with the relevant code context or file information.tests/unit/openclaw-editing.test.mjs (1)
1-3: The harness already injectstestinto the global scope—no import needed.
tests/unit/run.mjsdefinesglobalThis.test = (name, fn) => ...before importing all*.test.mjsfiles. This explicit harness injection makes thetestfunction available to all unit test modules without requiring an explicit import fromnode:test. The existing code is correct as-is.> Likely an incorrect or invalid review comment.
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 `@web-ui/modules/app.methods.openclaw-core.mjs`:
- Around line 98-123: The function readFirstProviderDisplayValue returns
env-template values with kind 'env-template' but leaves readOnly false; update
the branch that detects isEnvTemplateString(record[key]) inside
readFirstProviderDisplayValue so that when an env template is detected it
returns readOnly: true (i.e., set readOnly to true for the env-template case),
using the existing helpers isEnvTemplateString and formatSecretRefLabel to
locate the logic to change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 64ac6bd3-2c10-4cb8-a154-6933b704a61c
📒 Files selected for processing (4)
tests/unit/openclaw-core.test.mjstests/unit/openclaw-editing.test.mjsweb-ui/modules/app.methods.openclaw-core.mjsweb-ui/partials/index/panel-config-openclaw.html
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/unit/openclaw-editing.test.mjs
- tests/unit/openclaw-core.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-04-04T01:36:26.964Z
Learning: In PR `#68` for SakuraByteCore/codexmate, github-actions[bot] again flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint (reinforced across PRs `#55`, `#59`, `#67`, `#68`) is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-04-03T10:42:22.368Z
Learning: In PR `#67` for SakuraByteCore/codexmate, github-actions[bot] flagged that CodeRabbit was again introducing breaking changes. The maintainer's absolute constraint (reinforced across PRs `#55`, `#59`, `#67`) is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
🔇 Additional comments (7)
web-ui/partials/index/panel-config-openclaw.html (1)
68-68: Good guard on default-config deletion.This conditional render avoids exposing a delete action for
默认配置, which keeps UI behavior consistent with the protected default-config flow.web-ui/modules/app.methods.openclaw-core.mjs (6)
1-54: LGTM: Helper functions for provider normalization are well-structured.The
isPlainRecord,normalizeProviderId,findNormalizedProviderKey, andcollectDistinctProviderKeysfunctions handle edge cases properly (null checks, empty strings) and the provider aliasing logic is comprehensive.
200-217: LGTM: Quick defaults extended with read-only flags.The addition of
baseUrlReadOnlyandapiKeyReadOnlyproperties integrates cleanly with the UI template bindings shown in the context snippet.
227-343: LGTM: Refactored config parsing is more robust.The
fillOpenclawQuickFromConfigfunction now:
- Uses
isPlainRecordfor consistent object validation- Normalizes provider names for cross-location matching
- Auto-selects when only one provider/model exists
- Properly propagates
readOnlyflags for structured references- Supports both camelCase and snake_case field names
The past issue with
preferred_auth_methodbeing treated as an API key has been addressed (no longer in the key list at line 322).
521-543: LGTM: Provider value formatting now handles structured references.The additions correctly format secret refs as
SecretRef(...)and env templates asEnvRef(...)for display purposes. The early return for secret refs (lines 525-527) prevents further processing, which is appropriate.
125-132: LGTM: Helper for extracting provider models is straightforward.
56-58: Env template regex restriction is intentional and appropriate.The uppercase-only requirement (
/^\$\{[A-Z][A-Z0-9_]{0,127}\}$/) serves to enforce a clear naming convention that distinguishes environment variable references (${UPPERCASE}) from regular JavaScript template literals (${anything}). This classification is used at lines 106 and 541 to mark values as'env-template'kind and display them asEnvRef(...)in the UI. All actual environment variables in the codebase (e.g.,OPENAI_API_KEY,ANTHROPIC_API_KEY) follow the UPPERCASE convention, confirming this is the intended design.
| function readFirstProviderDisplayValue(records, keys) { | ||
| for (const record of records) { | ||
| if (!isPlainRecord(record)) continue; | ||
| for (const key of keys) { | ||
| if (typeof record[key] === 'string' && record[key].trim()) { | ||
| return { | ||
| value: record[key].trim(), | ||
| readOnly: false, | ||
| kind: isEnvTemplateString(record[key]) ? 'env-template' : 'string' | ||
| }; | ||
| } | ||
| if (coerceSecretRefRecord(record[key])) { | ||
| return { | ||
| value: formatSecretRefLabel(record[key]), | ||
| readOnly: true, | ||
| kind: 'secret-ref' | ||
| }; | ||
| } | ||
| } | ||
| } | ||
| return { | ||
| value: '', | ||
| readOnly: false, | ||
| kind: 'missing' | ||
| }; | ||
| } |
There was a problem hiding this comment.
Env template values should be read-only to prevent accidental overwriting.
When a value is an env template (e.g., ${API_URL}), the function correctly identifies it via kind: 'env-template' but still sets readOnly: false. Based on the context snippet from modal-openclaw-config.html, the UI hint says "检测到结构化引用" for read-only values, implying env templates are also structured references that shouldn't be editable in the quick form.
Without read-only protection, users could accidentally overwrite an env template reference with literal text, corrupting the configuration.
🛡️ Proposed fix to make env templates read-only
function readFirstProviderDisplayValue(records, keys) {
for (const record of records) {
if (!isPlainRecord(record)) continue;
for (const key of keys) {
if (typeof record[key] === 'string' && record[key].trim()) {
+ const isEnvRef = isEnvTemplateString(record[key]);
return {
value: record[key].trim(),
- readOnly: false,
- kind: isEnvTemplateString(record[key]) ? 'env-template' : 'string'
+ readOnly: isEnvRef,
+ kind: isEnvRef ? 'env-template' : 'string'
};
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-ui/modules/app.methods.openclaw-core.mjs` around lines 98 - 123, The
function readFirstProviderDisplayValue returns env-template values with kind
'env-template' but leaves readOnly false; update the branch that detects
isEnvTemplateString(record[key]) inside readFirstProviderDisplayValue so that
when an env template is detected it returns readOnly: true (i.e., set readOnly
to true for the env-template case), using the existing helpers
isEnvTemplateString and formatSecretRefLabel to locate the logic to change.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
cli.js (2)
11041-11050:⚠️ Potential issue | 🟠 MajorGuard
paramsand pass the normalizedsourcethrough.This branch still throws when the request omits
params, and mixed-case values like" Codex "validate here but fall back toallbecauselistSessionUsage()receives the raw object.🔧 Minimal fix
case 'list-sessions-usage': { - const source = typeof params.source === 'string' ? params.source.trim().toLowerCase() : ''; + const safeParams = params && typeof params === 'object' && !Array.isArray(params) ? params : {}; + const source = typeof safeParams.source === 'string' ? safeParams.source.trim().toLowerCase() : ''; if (source && source !== 'codex' && source !== 'claude' && source !== 'all') { result = { error: 'Invalid source. Must be codex, claude, or all' }; } else { result = { - sessions: await listSessionUsage(params || {}), + sessions: await listSessionUsage({ ...safeParams, source: source || 'all' }), source: source || 'all' }; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 11041 - 11050, In the 'case "list-sessions-usage"' branch guard against params being undefined before accessing params.source and create a normalized source string (trim().toLowerCase()) reliably; then pass a params object into listSessionUsage that includes the normalized source (e.g., overwrite or set params.source to the normalized value) so mixed-case/whitespace inputs are handled and listSessionUsage receives the corrected value instead of the raw original.
5763-5774:⚠️ Potential issue | 🟠 MajorHydrate exact counts before dropping
__messageCountExact.This still builds usage data from
listAllSessions(), then strips the only signal thatmessageCountmay be approximate. Larger sessions will be undercounted in the usage view.🔧 Minimal fix
- const sessions = await listAllSessions({ + const sessions = await listAllSessionsData({ source, limit, forceRefresh: !!params.forceRefresh }); - return sessions.map((item) => { - if (!item || typeof item !== 'object' || Array.isArray(item)) { - return item; - } - const normalized = { ...item }; - delete normalized.__messageCountExact; - return normalized; - }); + return sessions;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 5763 - 5774, The map after listAllSessions is dropping __messageCountExact before using it to hydrate messageCount, causing undercounts; update the mapped transformation (inside the sessions.map callback where `item`, `normalized`, and `__messageCountExact` are referenced) to first set normalized.messageCount = item.__messageCountExact (or use it to correct/override any approximate messageCount) when present, then delete normalized.__messageCountExact and return normalized so exact counts are preserved in the usage view.
🧹 Nitpick comments (4)
web-ui/modules/app.methods.openclaw-persist.mjs (2)
314-317: Consider explicit return for consistency.
deleteOpenclawConfigreturns the result ofshowMessage()for the default config guard, butshowMessagetypically returnsundefined. While this works (returns early), an explicitreturn;after the message would be clearer.🔧 Suggested fix
async deleteOpenclawConfig(name) { if (name === DEFAULT_OPENCLAW_CONFIG_NAME) { - return this.showMessage('默认配置始终映射当前系统配置，不可删除', 'info'); + this.showMessage('默认配置始终映射当前系统配置，不可删除', 'info'); + return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/modules/app.methods.openclaw-persist.mjs` around lines 314 - 317, In deleteOpenclawConfig, make the early-exit guard explicit: call this.showMessage(...) (await it if showMessage is async) to display the info, then explicitly return; instead of relying on showMessage's implicit undefined return; reference: function deleteOpenclawConfig and constant DEFAULT_OPENCLAW_CONFIG_NAME and method showMessage.
54-80: Inconsistent indentation insidesyncDefaultOpenclawConfigEntry.The
syncDefaultOpenclawConfigStatecall (lines 65-70) and the closing brace on line 71 have inconsistent indentation relative to the surrounding code. While this doesn't break functionality, it reduces readability.🔧 Suggested formatting fix
return api('get-openclaw-config') .then((res) => { - if (res && !res.error) { - this.openclawAuthProfilesByProvider = res && res.authProfilesByProvider && typeof res.authProfilesByProvider === 'object' && !Array.isArray(res.authProfilesByProvider) - ? res.authProfilesByProvider - : {}; - const nextContent = res.exists && typeof res.content === 'string' && res.content.trim() - ? res.content - : defaultOpenclawTemplate; - syncDefaultOpenclawConfigState(this, nextContent, { - path: res.path || this.openclawConfigPath, - exists: !!res.exists, - lineEnding: res.lineEnding === '\r\n' ? '\r\n' : '\n', - persist: true - }); + if (res && !res.error) { + this.openclawAuthProfilesByProvider = res && res.authProfilesByProvider && typeof res.authProfilesByProvider === 'object' && !Array.isArray(res.authProfilesByProvider) + ? res.authProfilesByProvider + : {}; + const nextContent = res.exists && typeof res.content === 'string' && res.content.trim() + ? res.content + : defaultOpenclawTemplate; + syncDefaultOpenclawConfigState(this, nextContent, { + path: res.path || this.openclawConfigPath, + exists: !!res.exists, + lineEnding: res.lineEnding === '\r\n' ? '\r\n' : '\n', + persist: true + }); } return res; })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/modules/app.methods.openclaw-persist.mjs` around lines 54 - 80, The syncDefaultOpenclawConfigEntry function has inconsistent indentation around the syncDefaultOpenclawConfigState call and its closing brace; re-indent the block so the syncDefaultOpenclawConfigState(...) invocation and the closing brace align with the surrounding if (res && !res.error) { ... } block (match the same indentation level used for this.openclawAuthProfilesByProvider assignment), ensuring the options object (path/exists/lineEnding/persist) is consistently indented and the if-block’s closing brace lines up with the if statement; verify no other lines in syncDefaultOpenclawConfigEntry are shifted out of alignment.web-ui/app.js (1)
378-397: Consider extracting shared normalization logic to avoid duplication.The
normalizeOpenclawConfigshelper here duplicates the logic inbuildNormalizedOpenclawConfigsfromapp.methods.openclaw-persist.mjs. Both functions perform the same normalization: ensuring a'默认配置'entry exists with validcontent.Consider importing and reusing the shared function to maintain a single source of truth.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/app.js` around lines 378 - 397, Duplicate normalization logic: replace the local normalizeOpenclawConfigs with a single shared implementation by importing and using buildNormalizedOpenclawConfigs from app.methods.openclaw-persist.mjs; update calls to use buildNormalizedOpenclawConfigs (or re-export it under the same name if needed), remove the local normalizeOpenclawConfigs function, and ensure the import path and exported symbol match the module so that the same validation (ensuring '默认配置' exists with valid content) is applied consistently across both files.tests/unit/openclaw-editing.test.mjs (1)
203-205: Verify hardcoded baseUrl expectation.The assertion on line 204 expects
baseUrlto be'https://chatgpt.com/backend-api', which appears to be a builtin default for the'openai-codex'provider. If this default changes in the future, this test will break.Consider either:
- Adding a comment explaining this is the expected builtin default
- Asserting it's a non-empty string if the exact value isn't critical to the test's purpose
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/openclaw-editing.test.mjs` around lines 203 - 205, The test tightly asserts parsed.data.models.providers['openai-codex'].baseUrl equals the hardcoded 'https://chatgpt.com/backend-api', which will break if the provider default changes; update the test to either add a clear comment above the assertion stating this is an intentional builtin default for 'openai-codex', or relax the assertion to only verify that parsed.data.models.providers['openai-codex'].baseUrl is a non-empty string (e.g., check typeof === 'string' and length > 0) so the test focuses on presence/format rather than an exact literal; adjust the assertion near parsed.data.models.providers['openai-codex'].baseUrl and leave the surrounding check of context.openclawPendingAuthProfileUpdates unchanged.
🤖 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 3309-3318: The code applies auth profile changes via
applyOpenclawAuthProfileUpdates(...) before backing up/writing
OPENCLAW_CONFIG_FILE, which can leave auth-profiles.json mutated if the later
fs.writeFileSync fails; change the flow to either stage both writes and only
commit them after both succeed (e.g., create backups via backupFileIfNeededOnce
for both OPENCLAW_CONFIG_FILE and the auth store, write to temp files and rename
on success) or, if keeping current ordering, catch failures after
fs.writeFileSync and restore the auth store using authUpdateResult.backupPath
(call the appropriate restore routine to roll back
applyOpenclawAuthProfileUpdates), ensureDir is used before staging, and surface
a clear composite error on failure.
- Around line 2847-2849: Remove the duplicate isPlainObject function declaration
(the second one) so only the original helper remains; locate the redundant
function named isPlainObject in the later part of the file (the declaration
matching "function isPlainObject(value) { return !!(value && typeof value ===
'object' && !Array.isArray(value)); }") and delete that block, leaving the
original isPlainObject definition intact and ensuring no other changes to
references or exports are made.
---
Duplicate comments:
In `@cli.js`:
- Around line 11041-11050: In the 'case "list-sessions-usage"' branch guard
against params being undefined before accessing params.source and create a
normalized source string (trim().toLowerCase()) reliably; then pass a params
object into listSessionUsage that includes the normalized source (e.g.,
overwrite or set params.source to the normalized value) so mixed-case/whitespace
inputs are handled and listSessionUsage receives the corrected value instead of
the raw original.
- Around line 5763-5774: The map after listAllSessions is dropping
__messageCountExact before using it to hydrate messageCount, causing
undercounts; update the mapped transformation (inside the sessions.map callback
where `item`, `normalized`, and `__messageCountExact` are referenced) to first
set normalized.messageCount = item.__messageCountExact (or use it to
correct/override any approximate messageCount) when present, then delete
normalized.__messageCountExact and return normalized so exact counts are
preserved in the usage view.
---
Nitpick comments:
In `@tests/unit/openclaw-editing.test.mjs`:
- Around line 203-205: The test tightly asserts
parsed.data.models.providers['openai-codex'].baseUrl equals the hardcoded
'https://chatgpt.com/backend-api', which will break if the provider default
changes; update the test to either add a clear comment above the assertion
stating this is an intentional builtin default for 'openai-codex', or relax the
assertion to only verify that
parsed.data.models.providers['openai-codex'].baseUrl is a non-empty string
(e.g., check typeof === 'string' and length > 0) so the test focuses on
presence/format rather than an exact literal; adjust the assertion near
parsed.data.models.providers['openai-codex'].baseUrl and leave the surrounding
check of context.openclawPendingAuthProfileUpdates unchanged.
In `@web-ui/app.js`:
- Around line 378-397: Duplicate normalization logic: replace the local
normalizeOpenclawConfigs with a single shared implementation by importing and
using buildNormalizedOpenclawConfigs from app.methods.openclaw-persist.mjs;
update calls to use buildNormalizedOpenclawConfigs (or re-export it under the
same name if needed), remove the local normalizeOpenclawConfigs function, and
ensure the import path and exported symbol match the module so that the same
validation (ensuring '默认配置' exists with valid content) is applied consistently
across both files.
In `@web-ui/modules/app.methods.openclaw-persist.mjs`:
- Around line 314-317: In deleteOpenclawConfig, make the early-exit guard
explicit: call this.showMessage(...) (await it if showMessage is async) to
display the info, then explicitly return; instead of relying on showMessage's
implicit undefined return; reference: function deleteOpenclawConfig and constant
DEFAULT_OPENCLAW_CONFIG_NAME and method showMessage.
- Around line 54-80: The syncDefaultOpenclawConfigEntry function has
inconsistent indentation around the syncDefaultOpenclawConfigState call and its
closing brace; re-indent the block so the syncDefaultOpenclawConfigState(...)
invocation and the closing brace align with the surrounding if (res &&
!res.error) { ... } block (match the same indentation level used for
this.openclawAuthProfilesByProvider assignment), ensuring the options object
(path/exists/lineEnding/persist) is consistently indented and the if-block’s
closing brace lines up with the if statement; verify no other lines in
syncDefaultOpenclawConfigEntry are shifted out of alignment.
🪄 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: 6cb9f9ad-9473-4c73-a3f1-1a4cbdf2e90e
📒 Files selected for processing (10)
cli.jstests/unit/openclaw-core.test.mjstests/unit/openclaw-editing.test.mjstests/unit/openclaw-persist-regression.test.mjstests/unit/web-ui-behavior-parity.test.mjsweb-ui/app.jsweb-ui/modules/app.methods.openclaw-core.mjsweb-ui/modules/app.methods.openclaw-editing.mjsweb-ui/modules/app.methods.openclaw-persist.mjsweb-ui/partials/index/modal-openclaw-config.html
✅ Files skipped from review due to trivial changes (1)
- tests/unit/openclaw-core.test.mjs
🚧 Files skipped from review as they are similar to previous changes (2)
- web-ui/modules/app.methods.openclaw-editing.mjs
- web-ui/modules/app.methods.openclaw-core.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-04-04T01:36:26.964Z
Learning: In PR `#68` for SakuraByteCore/codexmate, github-actions[bot] again flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint (reinforced across PRs `#55`, `#59`, `#67`, `#68`) is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
🪛 GitHub Actions: ci
cli.js
[error] 2847-2847: Lint failed due to SyntaxError: Identifier 'isPlainObject' has already been declared.
🔇 Additional comments (12)
cli.js (1)
3756-3813: LGTM: normalized URLs are now persisted consistently.The add/update flows now validate and write the same canonical base URL across the web and CLI paths, which avoids the old raw-vs-normalized mismatch.
Also applies to: 8570-8605, 8651-8813
web-ui/app.js (2)
119-124: Rename__mainTabSwitchState— underscore-prefixeddatakeys are not proxied tothisin Vue 3.In Vue 3's Options API,
data()properties starting with_or$are made reactive but are not proxied onto the component instance. This meansthis.__mainTabSwitchStatewill beundefinedin methods and templates, requiring access viathis.$data.__mainTabSwitchStateinstead.Rename to a non-reserved name like
mainTabSwitchStateortabSwitchState.
413-414: No race condition risk—the two operations use separate state domains.
syncDefaultOpenclawConfigEntry()returns a fire-and-forget Promise that updates openclaw-specific state (openclawAuthProfilesByProviderandopenclawConfigs), whileloadAll()loads provider/codex state via different API calls (status,list,list-auth-profiles). These functions operate on completely independent state, so the lack of awaiting does not create a race condition. The fire-and-forget pattern here is acceptable if parallel execution is intentional.web-ui/modules/app.methods.openclaw-persist.mjs (1)
286-290: LGTM!The
authProfileUpdatesparameter is correctly forwarded to the API usingObject.values()to convert the keyed object into an array. This aligns with the backend'sapplyOpenclawAuthProfileUpdateshandler (per context snippet 2) which expects an array of update entries.web-ui/partials/index/modal-openclaw-config.html (1)
265-272: LGTM!The conditional warning message and disabled state for the "保存" button correctly implement the default config save restriction. Users are clearly informed that the default config requires "保存并应用" while the button is appropriately disabled.
tests/unit/web-ui-behavior-parity.test.mjs (2)
322-334: LGTM!The expanded allowlists correctly document the new data keys introduced in this PR:
__mainTabSwitchStatefor tab switching stateopenclawAuthProfilesByProviderandopenclawPendingAuthProfileUpdatesfor auth profile handlingsessionsUsage*fields for session usage trackingThe conditional handling between
parityAgainstHeadand non-HEAD paths maintains backward compatibility while allowing the new keys.
360-372: LGTM!The new method keys accurately reflect the provider validation methods added in this PR as mentioned in the objectives (field-level validation, normalization). The allowlist includes all the expected methods:
canSubmitProvider,getProviderValidation,isValidProviderNameInput,isValidProviderUrlInput,normalizeProviderDraft, etc.tests/unit/openclaw-editing.test.mjs (2)
1-51: LGTM!The test setup is well-structured:
- Proper module imports using dynamic
import()with path resolutioncreateContexthelper provides a complete mock context with all required methods and state- Default state values align with the production code structure
53-109: LGTM!This test correctly verifies that structured provider references (using
source: 'env'objects forbaseUrlandapiKey) are preserved through the quick-to-text application flow rather than being stringified. This is important for maintaining external configuration references.tests/unit/openclaw-persist-regression.test.mjs (3)
126-158: LGTM!This test correctly verifies that editing the default config (
'默认配置') always fetches fresh content from the API rather than using stale local state. The assertions confirm:
- Content is refreshed from API response
- Path and exists state are updated
currentOpenclawConfigremains unchanged (good isolation)
437-478: LGTM!This test provides excellent coverage for the auth profile update forwarding:
- Verifies the pending updates object is correctly converted to an array via
Object.values()- Confirms the update entry structure matches backend expectations (
profileId,provider,field,value)- Verifies pending updates are cleared after successful apply
308-325: LGTM!The test correctly verifies the guard preventing local-only save for the default config when
lockNameis true. The assertion confirms an info message is shown guiding users to use "保存并应用" instead.
| const authUpdateResult = applyOpenclawAuthProfileUpdates({ updates: params.authProfileUpdates }); | ||
| if (!authUpdateResult.ok) { | ||
| return { | ||
| success: false, | ||
| error: authUpdateResult.error || '写入 OpenClaw 认证配置失败' | ||
| }; | ||
| } | ||
| ensureDir(OPENCLAW_DIR); | ||
| const backupPath = backupFileIfNeededOnce(OPENCLAW_CONFIG_FILE); | ||
| fs.writeFileSync(OPENCLAW_CONFIG_FILE, normalized, 'utf-8'); |
There was a problem hiding this comment.
Avoid partial saves when the OpenClaw config write fails.
applyOpenclawAuthProfileUpdates() is committed before openclaw.json is backed up/written, so any later filesystem error returns failure after auth-profiles.json has already changed. Please roll authStorePath back in the catch path via authUpdateResult.backupPath, or stage both writes before committing either file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli.js` around lines 3309 - 3318, The code applies auth profile changes via
applyOpenclawAuthProfileUpdates(...) before backing up/writing
OPENCLAW_CONFIG_FILE, which can leave auth-profiles.json mutated if the later
fs.writeFileSync fails; change the flow to either stage both writes and only
commit them after both succeed (e.g., create backups via backupFileIfNeededOnce
for both OPENCLAW_CONFIG_FILE and the auth store, write to temp files and rename
on success) or, if keeping current ordering, catch failures after
fs.writeFileSync and restore the auth store using authUpdateResult.backupPath
(call the appropriate restore routine to roll back
applyOpenclawAuthProfileUpdates), ensureDir is used before staging, and surface
a clear composite error on failure.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli.js (1)
2829-2840:⚠️ Potential issue | 🔴 CriticalDo not ship
resolvedValueback inget-openclaw-config.
readOpenclawConfigFile()now forwardsauthProfilesByProvider, and that summary includes the live credential string for editable profiles. That turns a read-config call into a raw secret disclosure path; please strip or mask the value in this response and keep the real token only in the write flow.Also applies to: 3037-3040
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 2829 - 2840, The response currently returns authProfilesByProvider from readOpenclawAuthProfilesSummary() which includes live credential strings (resolvedValue); in readOpenclawConfigFile (the code path returning the object with authProfilesByProvider) map over the providers and strip or replace each profile's resolvedValue (e.g., set to null/"[REDACTED]" or delete the key) before placing it on the returned object so the live token is not shipped on read; ensure the same masking is applied to the other return path referenced (lines ~3037-3040) and keep the real resolvedValue only in the write flow.
♻️ Duplicate comments (3)
cli.js (3)
11039-11045:⚠️ Potential issue | 🟠 MajorGuard missing
paramsand pass the normalized source through.This block still reads
params.sourcebefore provingparamsis an object, so{ "action": "list-sessions-usage" }throws a 500. It also validates a normalizedsourcebut then callslistSessionUsage(params || {}), so mixed-case or space-padded inputs still fall back toall.🛡️ Minimal fix
- const source = typeof params.source === 'string' ? params.source.trim().toLowerCase() : ''; + const safeParams = params && typeof params === 'object' && !Array.isArray(params) ? params : {}; + const source = typeof safeParams.source === 'string' ? safeParams.source.trim().toLowerCase() : ''; if (source && source !== 'codex' && source !== 'claude' && source !== 'all') { result = { error: 'Invalid source. Must be codex, claude, or all' }; } else { result = { - sessions: await listSessionUsage(params || {}), + sessions: await listSessionUsage({ ...safeParams, source: source || 'all' }), source: source || 'all' }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 11039 - 11045, Guard against params being null/undefined and use the normalized source value when calling listSessionUsage: ensure params is defaulted to an object (e.g., const p = params || {}), compute const source = typeof p.source === 'string' ? p.source.trim().toLowerCase() : '' and validate that source is one of 'codex'|'claude'|'all', set result.error on invalid, and when valid call listSessionUsage with the adjusted params (e.g., pass {...p, source: source || 'all'}) and set result.source to the normalized value so mixed-case/space-padded inputs are honored; update references to params/source accordingly (look for params, source, and listSessionUsage).
3305-3332:⚠️ Potential issue | 🟠 MajorRoll back auth-profile writes if
openclaw.jsonfails.
applyOpenclawAuthProfileUpdates()commits beforeOPENCLAW_CONFIG_FILEis written, so any later filesystem error returns failure afterauth-profiles.jsonhas already changed. Please restore fromauthUpdateResult.backupPathin the catch path, or delete the newly created auth store when there was no original file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 3305 - 3332, When writing OPENCLAW_CONFIG_FILE fails after calling applyOpenclawAuthProfileUpdates, undo the partial auth-store change: in the catch block check authUpdateResult; if authUpdateResult.backupPath exists restore the original auth store from that path (overwrite authUpdateResult.authStorePath), otherwise if authUpdateResult.authStorePath exists and there was no prior backup delete the new auth store file; ensure any restore/delete errors are caught/logged but do not mask the original error being returned.
5759-5770:⚠️ Potential issue | 🟠 MajorHydrate exact counts before stripping
__messageCountExact.
listAllSessions()still produces approximatemessageCountvalues for larger session files. Deleting the exactness flag here exposes undercounted usage data instead of the fully hydrated counts already available vialistAllSessionsData().📊 Minimal fix
- const sessions = await listAllSessions({ + const sessions = await listAllSessionsData({ source, limit, forceRefresh: !!params.forceRefresh }); - return sessions.map((item) => { - if (!item || typeof item !== 'object' || Array.isArray(item)) { - return item; - } - const normalized = { ...item }; - delete normalized.__messageCountExact; - return normalized; - }); + return sessions;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 5759 - 5770, The code currently deletes the __messageCountExact flag from each session returned by listAllSessions, which can expose undercounted approximate messageCount values; fix this by calling listAllSessionsData(...) with the same parameters (source, limit, forceRefresh) to obtain fully hydrated session records, build a lookup by session id, and for each item in the sessions.map() (inside the iterator over listAllSessions results) replace/merge the messageCount (and any other hydrated fields) from the corresponding listAllSessionsData record before deleting __messageCountExact so the output contains the fully hydrated counts; reference listAllSessions, listAllSessionsData, __messageCountExact, and messageCount when implementing the merge.
🤖 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 3058-3062: The current validation accepts any of
['key','token','access'] and then rewrites the profile type; instead, fetch the
existing profile (variable often named existing) and call
resolveOpenclawAuthProfileEditableValue(existing) to determine the allowed
editable field for that profile, then compare that result to the incoming field
and return null (reject) if they differ; apply the same guard in both the block
around the allowedFields check (profileId, provider, field, value) and the
analogous block at 3083-3104 so clients cannot change an api_key profile into
token/oauth or vice versa.
---
Outside diff comments:
In `@cli.js`:
- Around line 2829-2840: The response currently returns authProfilesByProvider
from readOpenclawAuthProfilesSummary() which includes live credential strings
(resolvedValue); in readOpenclawConfigFile (the code path returning the object
with authProfilesByProvider) map over the providers and strip or replace each
profile's resolvedValue (e.g., set to null/"[REDACTED]" or delete the key)
before placing it on the returned object so the live token is not shipped on
read; ensure the same masking is applied to the other return path referenced
(lines ~3037-3040) and keep the real resolvedValue only in the write flow.
---
Duplicate comments:
In `@cli.js`:
- Around line 11039-11045: Guard against params being null/undefined and use the
normalized source value when calling listSessionUsage: ensure params is
defaulted to an object (e.g., const p = params || {}), compute const source =
typeof p.source === 'string' ? p.source.trim().toLowerCase() : '' and validate
that source is one of 'codex'|'claude'|'all', set result.error on invalid, and
when valid call listSessionUsage with the adjusted params (e.g., pass {...p,
source: source || 'all'}) and set result.source to the normalized value so
mixed-case/space-padded inputs are honored; update references to params/source
accordingly (look for params, source, and listSessionUsage).
- Around line 3305-3332: When writing OPENCLAW_CONFIG_FILE fails after calling
applyOpenclawAuthProfileUpdates, undo the partial auth-store change: in the
catch block check authUpdateResult; if authUpdateResult.backupPath exists
restore the original auth store from that path (overwrite
authUpdateResult.authStorePath), otherwise if authUpdateResult.authStorePath
exists and there was no prior backup delete the new auth store file; ensure any
restore/delete errors are caught/logged but do not mask the original error being
returned.
- Around line 5759-5770: The code currently deletes the __messageCountExact flag
from each session returned by listAllSessions, which can expose undercounted
approximate messageCount values; fix this by calling listAllSessionsData(...)
with the same parameters (source, limit, forceRefresh) to obtain fully hydrated
session records, build a lookup by session id, and for each item in the
sessions.map() (inside the iterator over listAllSessions results) replace/merge
the messageCount (and any other hydrated fields) from the corresponding
listAllSessionsData record before deleting __messageCountExact so the output
contains the fully hydrated counts; reference listAllSessions,
listAllSessionsData, __messageCountExact, and messageCount when implementing the
merge.
🪄 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: 2c9537fa-4218-40f8-b767-da5824b77a0d
📒 Files selected for processing (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-04-04T01:36:26.964Z
Learning: In PR `#68` for SakuraByteCore/codexmate, github-actions[bot] again flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint (reinforced across PRs `#55`, `#59`, `#67`, `#68`) is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-31T12:37:51.023Z
Learning: In PR `#59` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-04-03T10:42:22.368Z
Learning: In PR `#67` for SakuraByteCore/codexmate, github-actions[bot] flagged that CodeRabbit was again introducing breaking changes. The maintainer's absolute constraint (reinforced across PRs `#55`, `#59`, `#67`) is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-30T04:47:15.352Z
Learning: In PR `#55` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
🔇 Additional comments (1)
cli.js (1)
3757-3808: Nice consistency win on provider URL normalization.The add/update flows now validate and persist the same normalized base URL, so trailing-slash cleanup is no longer lost between validation and write across both the API and CLI paths.
Also applies to: 3845-3847, 8566-8601, 8649-8809
| const allowedFields = new Set(['key', 'token', 'access']); | ||
| if (!profileId || !provider || !allowedFields.has(field) || !value) { | ||
| return null; | ||
| } | ||
| return { profileId, provider, field, value }; |
There was a problem hiding this comment.
Reject auth-profile updates that target the wrong credential field.
The server currently accepts any of key|token|access for any existing profile and then rewrites type to match the request. A stale or tampered client can silently convert an api_key profile into token/oauth and persist a record the UI never exposed. Compare the requested field against resolveOpenclawAuthProfileEditableValue(existing) before mutating.
🔒 Minimal guard
const existingProvider = typeof existing.provider === 'string' ? existing.provider.trim() : '';
if (normalizeOpenclawProviderId(existingProvider) !== normalizeOpenclawProviderId(update.provider)) {
return { ok: false, error: `认证配置与 provider 不匹配: ${update.profileId}` };
}
+ const editableMeta = resolveOpenclawAuthProfileEditableValue(existing);
+ if (!editableMeta.editable || editableMeta.resolvedField !== update.field) {
+ return { ok: false, error: `认证配置不支持更新字段: ${update.profileId}` };
+ }
const next = { ...existing };Also applies to: 3083-3104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli.js` around lines 3058 - 3062, The current validation accepts any of
['key','token','access'] and then rewrites the profile type; instead, fetch the
existing profile (variable often named existing) and call
resolveOpenclawAuthProfileEditableValue(existing) to determine the allowed
editable field for that profile, then compare that result to the incoming field
and return null (reject) if they differ; apply the same guard in both the block
around the allowedFields check (profileId, provider, field, value) and the
analogous block at 3083-3104 so clients cannot change an api_key profile into
token/oauth or vice versa.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/web-ui-logic.test.mjs (1)
104-146:⚠️ Potential issue | 🔴 Critical
buildSessionListParamsassertions are ahead of implementation.At Line 104-146 and Line 592-627, expected defaults (
forceRefresh: falseand conditional fast/default limit behavior) do not matchweb-ui/logic.sessions.mjs:27-51, which currently useslimit = 200and returnsforceRefresh: true. This is a merge-blocking contract mismatch and is already failing CI.Also applies to: 592-627
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/web-ui-logic.test.mjs` around lines 104 - 146, The tests expect buildSessionListParams to trim/normalize source and pathFilter, default forceRefresh to false, and set limit to DEFAULT_SESSION_LIST_FAST_LIMIT only when source === 'all' and no pathFilter, no query, and forceRefresh is falsy; otherwise use DEFAULT_SESSION_LIST_LIMIT — update the buildSessionListParams implementation to (1) normalize source with .trim().toLowerCase() and pathFilter with .trim(), (2) set forceRefresh default to false, and (3) compute limit by checking if source === 'all' && !pathFilter && !query && !forceRefresh then DEFAULT_SESSION_LIST_FAST_LIMIT else DEFAULT_SESSION_LIST_LIMIT, keeping the other default fields (queryMode, queryScope, contentScanLimit, roleFilter, timeRangePreset) intact.
🤖 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-web-ui-session-tab.js`:
- Line 91: The test assertion expects a deferred tab commit but the mouse
pointerdown handler commits immediately; update the expectation to reflect that
onMainTabPointerDown calls switchMainTab(...) and mainTab is already "settings".
Change the assertion in tests/e2e/test-web-ui-session-tab.js around the
onMainTabPointerDown interaction to assert vm.mainTab === 'settings'
(referencing onMainTabPointerDown and switchMainTab) instead of expecting a
deferred 'sessions' value.
In `@tests/unit/web-ui-behavior-parity.test.mjs`:
- Around line 426-439: The non-HEAD branch is asserting equality against allowed
lists which is wrong for "missingCurrentComputedKeys" (it should be allowed to
be empty when keys are current-only); update the else branch to mirror the
parityAgainstHead logic: create allowedExtraCurrentComputedKeys and
allowedMissingCurrentComputedKeys sets (using allowedExtraCurrentComputedKeys
and allowedMissingCurrentComputedKeys), compute
unexpectedExtraCurrentComputedKeys = extraCurrentComputedKeys.filter(k =>
!allowedExtraSet.has(k)) and unexpectedMissingCurrentComputedKeys =
missingCurrentComputedKeys.filter(k => !allowedMissingSet.has(k)), then assert
both unexpected arrays are empty (like the if branch) so that current-only
additions such as sessionListRemainingCount and visibleSessionsList do not fail
the test.
In `@tests/unit/web-ui-logic.test.mjs`:
- Around line 13-14: DEFAULT_SESSION_LIST_FAST_LIMIT and
DEFAULT_SESSION_LIST_LIMIT are resolving to undefined in the tests; either
export them from the module that defines them or import them directly from their
defining module instead of relying on implicit availability. Update the source
module to export the constants (ensure symbols DEFAULT_SESSION_LIST_FAST_LIMIT
and DEFAULT_SESSION_LIST_LIMIT are exported) or change the test import to pull
these two named exports from the module where they're defined, then run tests to
confirm assertions use the real numeric values.
- Around line 641-653: The test assumes sortedSessionsList won't call
getSessionExportKey, but the implementation does compute keys; update the test
to stop throwing inside getSessionExportKey and instead provide a harmless stub
(e.g., return a deterministic string or record calls) so
createSessionComputed().sortedSessionsList can run without throwing; reference
the test helper createSessionComputed, the computed method sortedSessionsList,
and the stubbed getSessionExportKey when making this change.
---
Outside diff comments:
In `@tests/unit/web-ui-logic.test.mjs`:
- Around line 104-146: The tests expect buildSessionListParams to trim/normalize
source and pathFilter, default forceRefresh to false, and set limit to
DEFAULT_SESSION_LIST_FAST_LIMIT only when source === 'all' and no pathFilter, no
query, and forceRefresh is falsy; otherwise use DEFAULT_SESSION_LIST_LIMIT —
update the buildSessionListParams implementation to (1) normalize source with
.trim().toLowerCase() and pathFilter with .trim(), (2) set forceRefresh default
to false, and (3) compute limit by checking if source === 'all' && !pathFilter
&& !query && !forceRefresh then DEFAULT_SESSION_LIST_FAST_LIMIT else
DEFAULT_SESSION_LIST_LIMIT, keeping the other default fields (queryMode,
queryScope, contentScanLimit, roleFilter, timeRangePreset) intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 28cc35ae-72e9-48b3-9c9b-a4f84a21c50e
📒 Files selected for processing (6)
tests/e2e/test-web-ui-assets.jstests/e2e/test-web-ui-session-tab.jstests/unit/config-tabs-ui.test.mjstests/unit/web-ui-behavior-parity.test.mjstests/unit/web-ui-logic.test.mjstests/unit/web-ui-runtime-navigation-regression.test.mjs
✅ Files skipped from review due to trivial changes (1)
- tests/unit/web-ui-runtime-navigation-regression.test.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/config-tabs-ui.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-04-04T01:36:26.964Z
Learning: In PR `#68` for SakuraByteCore/codexmate, github-actions[bot] again flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint (reinforced across PRs `#55`, `#59`, `#67`, `#68`) is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-31T12:37:51.023Z
Learning: In PR `#59` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-30T04:47:15.352Z
Learning: In PR `#55` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-04-03T10:42:22.368Z
Learning: In PR `#67` for SakuraByteCore/codexmate, github-actions[bot] flagged that CodeRabbit was again introducing breaking changes. The maintainer's absolute constraint (reinforced across PRs `#55`, `#59`, `#67`) is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
🪛 GitHub Actions: ci
tests/unit/web-ui-behavior-parity.test.mjs
[error] 439-439: Unit test failure: captured bundled app skeleton only exposes expected data key drift versus parity baseline. AssertionError: expected keys ['sessionListRemainingCount','visibleSessionsList'] but actual was [].
tests/unit/web-ui-logic.test.mjs
[error] 105-105: Unit test failure: buildSessionListParams normalizes source/path filters and request params. AssertionError in buildSessionListParams normalizes source and path filter before building request: actual had forceRefresh: true and limit: 200 but expected forceRefresh: false and limit: undefined.
[error] 127-127: Unit test failure: buildSessionListParams uses fast limit only for default all-session browsing. AssertionError: 200 !== undefined.
[error] 592-592: Unit test failure: buildSessionListParams keeps query for enabled sources. AssertionError: 200 !== undefined.
[error] 626-626: Unit test failure: buildSessionListParams normalizes unsupported sources to all and preserves query behavior. AssertionError: 200 !== undefined.
[error] 648-648: Unit test failure: should not compute session keys when no pins exist. Error: should not compute session keys when no pins exist (thrown from getSessionExportKey).
🔇 Additional comments (5)
tests/e2e/test-web-ui-session-tab.js (1)
99-117: Good coverage for canceled deferred teardown on session re-entry.This block cleanly verifies re-priming (
prepareSessionTabRender) plus post-flush stability when returning tosessions.tests/e2e/test-web-ui-assets.js (1)
55-56: Solid asset-contract assertions for Vue runtime selection.These checks strengthen bundle regression coverage and match the current HTML source contract.
tests/unit/web-ui-behavior-parity.test.mjs (2)
322-357: Data-key drift handling looks correct.These allowlisted keys line up with the newly initialized session/navigation state in
web-ui/app.js:119-130, and theSet-based filtering keeps non-HEAD parity checks strict for unallowlisted drift only.
366-412: Method-key parity update looks consistent.The added allowlist matches the new session-rendering and usage-loading methods surfaced by
web-ui/modules/app.methods.navigation.mjs:412-435andweb-ui/modules/app.methods.session-browser.mjs:362-396, and the non-HEAD branch now follows the same unexpected-key pattern as the data section.tests/unit/web-ui-logic.test.mjs (1)
655-675: Pinned ordering coverage here is strong.Line 655-675 correctly verifies pinned-first ordering with latest pin timestamp precedence, while preserving expected relative placement for remaining sessions.
| }); | ||
|
|
||
| assert(vm.fastHidden === true, 'pointerdown should hide the sessions panel immediately'); | ||
| assert(vm.mainTab === 'sessions', 'tab commit should stay deferred until the next frame'); |
There was a problem hiding this comment.
Fix incorrect tab-state expectation after pointerdown commit.
Line 91 expects deferred commit, but onMainTabPointerDown for mouse input commits immediately via switchMainTab(...), so mainTab should already be "settings" here.
✅ Proposed fix
- assert(vm.mainTab === 'sessions', 'tab commit should stay deferred until the next frame');
+ assert(vm.mainTab === 'settings', 'mouse pointerdown should commit tab switch immediately');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert(vm.mainTab === 'sessions', 'tab commit should stay deferred until the next frame'); | |
| assert(vm.mainTab === 'settings', 'mouse pointerdown should commit tab switch immediately'); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/test-web-ui-session-tab.js` at line 91, The test assertion expects
a deferred tab commit but the mouse pointerdown handler commits immediately;
update the expectation to reflect that onMainTabPointerDown calls
switchMainTab(...) and mainTab is already "settings". Change the assertion in
tests/e2e/test-web-ui-session-tab.js around the onMainTabPointerDown interaction
to assert vm.mainTab === 'settings' (referencing onMainTabPointerDown and
switchMainTab) instead of expecting a deferred 'sessions' value.
| DEFAULT_SESSION_LIST_FAST_LIMIT, | ||
| DEFAULT_SESSION_LIST_LIMIT, |
There was a problem hiding this comment.
DEFAULT_SESSION_LIST_* imports are unresolved in current test run.
At Line 13 and Line 14, these constants are currently resolving to undefined (CI shows 200 !== undefined), so several limit assertions are invalid by construction. Please either export them from web-ui/logic.mjs or import from their defining module before relying on them in expectations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/web-ui-logic.test.mjs` around lines 13 - 14,
DEFAULT_SESSION_LIST_FAST_LIMIT and DEFAULT_SESSION_LIST_LIMIT are resolving to
undefined in the tests; either export them from the module that defines them or
import them directly from their defining module instead of relying on implicit
availability. Update the source module to export the constants (ensure symbols
DEFAULT_SESSION_LIST_FAST_LIMIT and DEFAULT_SESSION_LIST_LIMIT are exported) or
change the test import to pull these two named exports from the module where
they're defined, then run tests to confirm assertions use the real numeric
values.
|
@coderabbitai If the current suggestion is not adopted, will it lead to disruptive changes? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me check what suggestions were made on this PR first. 🏁 Script executed: Length of output: 263475
Must fix to avoid disruption
Should fix (robustness/UX correctness, still non-breaking)
Nice to have (safe to defer)
Bottom line
🧠 Learnings used |
|
@coderabbitai Please provide concrete, non-breaking patch guidance for these items on this PR:
Please keep the guidance limited to correctness and safety only, with no breaking changes or refactors. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 26768 🏁 Script executed: Length of output: 21490 🏁 Script executed: Length of output: 4606 🏁 Script executed: Length of output: 6479
1. Security —
|
|
@coderabbitai After commit de4ac6a, please check for any remaining non-breaking regression risks only. Please focus on:
If you still see possible regressions, please list only concrete correctness or safety risks, with the smallest non-breaking fix for each. |
There was a problem hiding this comment.
♻️ Duplicate comments (4)
cli.js (2)
3102-3123:⚠️ Potential issue | 🟠 MajorReject auth-profile updates that target a different credential field.
This loop still accepts any incoming
key|token|accessand then rewritesnext.typeto match it. A stale or tampered client can silently convert an existingapi_keyprofile intotoken/oauthand persist a shape the UI never exposed.Minimal guard
const existingProvider = typeof existing.provider === 'string' ? existing.provider.trim() : ''; if (normalizeOpenclawProviderId(existingProvider) !== normalizeOpenclawProviderId(update.provider)) { return { ok: false, error: `认证配置与 provider 不匹配: ${update.profileId}` }; } + const editableMeta = resolveOpenclawAuthProfileEditableValue(existing); + if (!editableMeta.editable || editableMeta.resolvedField !== update.field) { + return { ok: false, error: `认证配置不支持更新字段: ${update.profileId}` }; + } const next = { ...existing };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 3102 - 3123, The loop that applies entries from updates (iterating updates, using profiles[update.profileId], normalizeOpenclawProviderId, next and update.field) currently allows changing credential fields and blindly sets next.type based on update.field; instead, guard so an update that targets a different credential field than the existing credential type is rejected: determine the expected field for the existing entry (map existing.type -> allowed field: 'api_key'->'key', 'token'->'token', 'oauth'->'access') and if update.field does not match that allowed field, return an error (using the same { ok: false, error: ... } pattern referencing update.profileId); only when the fields match apply the change and set next.type as needed for new/initial cases.
3324-3352:⚠️ Potential issue | 🟠 MajorRoll back auth-profile writes if
openclaw.jsonfails.
applyOpenclawAuthProfileUpdates()is committed beforeOPENCLAW_CONFIG_FILEis written. If the later write throws, this returns failure afterauth-profiles.jsonhas already changed, leaving OpenClaw state split across two files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 3324 - 3352, The code currently calls applyOpenclawAuthProfileUpdates(...) before writing OPENCLAW_CONFIG_FILE so if fs.writeFileSync throws the auth store is left mutated; change the flow so either (A) write OPENCLAW_CONFIG_FILE first (using ensureDir and backupFileIfNeededOnce) and only call applyOpenclawAuthProfileUpdates(...) after the write succeeds, or (B) if you must call applyOpenclawAuthProfileUpdates(...) first, capture its result and on catch use that result (authUpdateResult.authStorePath and authUpdateResult.applied) to roll back the auth-store changes (restore backup at authStorePath or revert applied profiles) before returning the failure; update the try/catch around applyOpenclawAuthProfileUpdates, fs.writeFileSync, and backupFileIfNeededOnce accordingly and ensure returned result still populates authStorePath/applied on success.web-ui/modules/app.methods.openclaw-core.mjs (1)
109-118:⚠️ Potential issue | 🟡 MinorMark env-template values as read-only in quick form.
kind: 'env-template'is detected, butreadOnlyis stillfalse, so structured refs can be overwritten accidentally.🛠️ Minimal non-breaking patch
if (typeof record[key] === 'string' && record[key].trim()) { + const isEnvRef = isEnvTemplateString(record[key]); return { value: record[key].trim(), - readOnly: false, - kind: isEnvTemplateString(record[key]) ? 'env-template' : 'string' + readOnly: isEnvRef, + kind: isEnvRef ? 'env-template' : 'string' }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/modules/app.methods.openclaw-core.mjs` around lines 109 - 118, In readFirstProviderDisplayValue, when you detect an env template (isEnvTemplateString(record[key]) === true) mark the returned display object readOnly: true so structured refs cannot be edited; update the return so that for kind === 'env-template' you set readOnly to true (leave readOnly false for plain strings), referencing the function readFirstProviderDisplayValue and the isEnvTemplateString check to locate the change.tests/unit/web-ui-behavior-parity.test.mjs (1)
423-433:⚠️ Potential issue | 🟠 MajorComputed-key parity is still stricter in the non-HEAD branch.
Lines 432-433 still use exact equality, so the non-HEAD path can fail on allowed computed-key drift even though the data-key and method-key sections now correctly filter to unexpected differences only.
Based on learnings: 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.🛠 Minimal non-breaking fix
const allowedExtraCurrentComputedKeys = []; const allowedMissingCurrentComputedKeys = []; if (parityAgainstHead) { const allowedExtraComputedKeySet = new Set(allowedExtraCurrentComputedKeys); const allowedMissingComputedKeySet = new Set(allowedMissingCurrentComputedKeys); const unexpectedExtraCurrentComputedKeys = extraCurrentComputedKeys.filter((key) => !allowedExtraComputedKeySet.has(key)); const unexpectedMissingCurrentComputedKeys = missingCurrentComputedKeys.filter((key) => !allowedMissingComputedKeySet.has(key)); assert.deepStrictEqual(unexpectedExtraCurrentComputedKeys, [], `unexpected extra computed keys against ${parityBaseline.ref}`); assert.deepStrictEqual(unexpectedMissingCurrentComputedKeys, [], `unexpected missing computed keys against ${parityBaseline.ref}`); } else { - assert.deepStrictEqual(extraCurrentComputedKeys, allowedExtraCurrentComputedKeys); - assert.deepStrictEqual(missingCurrentComputedKeys, allowedMissingCurrentComputedKeys); + const allowedExtraComputedKeySet = new Set(allowedExtraCurrentComputedKeys); + const allowedMissingComputedKeySet = new Set(allowedMissingCurrentComputedKeys); + const unexpectedExtraCurrentComputedKeys = extraCurrentComputedKeys.filter((key) => !allowedExtraComputedKeySet.has(key)); + const unexpectedMissingCurrentComputedKeys = missingCurrentComputedKeys.filter((key) => !allowedMissingComputedKeySet.has(key)); + assert.deepStrictEqual(unexpectedExtraCurrentComputedKeys, [], `unexpected extra computed keys against ${parityBaseline.ref}`); + assert.deepStrictEqual(unexpectedMissingCurrentComputedKeys, [], `unexpected missing computed keys against ${parityBaseline.ref}`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/web-ui-behavior-parity.test.mjs` around lines 423 - 433, The non-HEAD branch currently asserts exact equality for computed-key arrays (using allowedExtraCurrentComputedKeys and allowedMissingCurrentComputedKeys) which is too strict; mirror the HEAD branch logic by creating sets from allowedExtraCurrentComputedKeys and allowedMissingCurrentComputedKeys, filter extraCurrentComputedKeys and missingCurrentComputedKeys into unexpectedExtraCurrentComputedKeys and unexpectedMissingCurrentComputedKeys (same as in the parityAgainstHead branch), and assert those filtered unexpected arrays are empty, using parityBaseline.ref in the failure messages to match the existing HEAD assertions.
🧹 Nitpick comments (1)
tests/e2e/test-openclaw.js (1)
6-8: Add a clear precondition fortmpHomein test setup.A small early assert gives clearer failures than a
path.joincrash when context wiring regresses.🧪 Small clarity patch
module.exports = async function testOpenclaw(ctx) { const { api, tmpHome } = ctx; + assert(typeof tmpHome === 'string' && tmpHome, 'testOpenclaw requires ctx.tmpHome'); const authAgentDir = path.join(tmpHome, '.openclaw', 'agents', 'main', 'agent');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/test-openclaw.js` around lines 6 - 8, Add an explicit precondition check for ctx.tmpHome before using path.join: verify ctx and tmpHome are present and truthy (e.g., assert(ctx && ctx.tmpHome) or throw a clear Error) immediately before computing authAgentDir so failures surface with a descriptive message instead of a path.join crash; update the setup around the symbols ctx, tmpHome, authAgentDir and the path.join/fs.mkdirSync sequence to rely on that early check.
🤖 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 3102-3123: The loop that applies entries from updates (iterating
updates, using profiles[update.profileId], normalizeOpenclawProviderId, next and
update.field) currently allows changing credential fields and blindly sets
next.type based on update.field; instead, guard so an update that targets a
different credential field than the existing credential type is rejected:
determine the expected field for the existing entry (map existing.type ->
allowed field: 'api_key'->'key', 'token'->'token', 'oauth'->'access') and if
update.field does not match that allowed field, return an error (using the same
{ ok: false, error: ... } pattern referencing update.profileId); only when the
fields match apply the change and set next.type as needed for new/initial cases.
- Around line 3324-3352: The code currently calls
applyOpenclawAuthProfileUpdates(...) before writing OPENCLAW_CONFIG_FILE so if
fs.writeFileSync throws the auth store is left mutated; change the flow so
either (A) write OPENCLAW_CONFIG_FILE first (using ensureDir and
backupFileIfNeededOnce) and only call applyOpenclawAuthProfileUpdates(...) after
the write succeeds, or (B) if you must call applyOpenclawAuthProfileUpdates(...)
first, capture its result and on catch use that result
(authUpdateResult.authStorePath and authUpdateResult.applied) to roll back the
auth-store changes (restore backup at authStorePath or revert applied profiles)
before returning the failure; update the try/catch around
applyOpenclawAuthProfileUpdates, fs.writeFileSync, and backupFileIfNeededOnce
accordingly and ensure returned result still populates authStorePath/applied on
success.
In `@tests/unit/web-ui-behavior-parity.test.mjs`:
- Around line 423-433: The non-HEAD branch currently asserts exact equality for
computed-key arrays (using allowedExtraCurrentComputedKeys and
allowedMissingCurrentComputedKeys) which is too strict; mirror the HEAD branch
logic by creating sets from allowedExtraCurrentComputedKeys and
allowedMissingCurrentComputedKeys, filter extraCurrentComputedKeys and
missingCurrentComputedKeys into unexpectedExtraCurrentComputedKeys and
unexpectedMissingCurrentComputedKeys (same as in the parityAgainstHead branch),
and assert those filtered unexpected arrays are empty, using parityBaseline.ref
in the failure messages to match the existing HEAD assertions.
In `@web-ui/modules/app.methods.openclaw-core.mjs`:
- Around line 109-118: In readFirstProviderDisplayValue, when you detect an env
template (isEnvTemplateString(record[key]) === true) mark the returned display
object readOnly: true so structured refs cannot be edited; update the return so
that for kind === 'env-template' you set readOnly to true (leave readOnly false
for plain strings), referencing the function readFirstProviderDisplayValue and
the isEnvTemplateString check to locate the change.
---
Nitpick comments:
In `@tests/e2e/test-openclaw.js`:
- Around line 6-8: Add an explicit precondition check for ctx.tmpHome before
using path.join: verify ctx and tmpHome are present and truthy (e.g., assert(ctx
&& ctx.tmpHome) or throw a clear Error) immediately before computing
authAgentDir so failures surface with a descriptive message instead of a
path.join crash; update the setup around the symbols ctx, tmpHome, authAgentDir
and the path.join/fs.mkdirSync sequence to rely on that early check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e0c5a22d-034d-4844-9664-65bbee20ffd6
📒 Files selected for processing (7)
cli.jstests/e2e/test-openclaw.jstests/e2e/test-sessions.jstests/unit/openclaw-core.test.mjstests/unit/openclaw-editing.test.mjstests/unit/web-ui-behavior-parity.test.mjsweb-ui/modules/app.methods.openclaw-core.mjs
✅ Files skipped from review due to trivial changes (2)
- tests/e2e/test-sessions.js
- tests/unit/openclaw-editing.test.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/openclaw-core.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-04-04T01:36:26.964Z
Learning: In PR `#68` for SakuraByteCore/codexmate, github-actions[bot] again flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint (reinforced across PRs `#55`, `#59`, `#67`, `#68`) is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-31T12:37:51.023Z
Learning: In PR `#59` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-30T04:47:15.352Z
Learning: In PR `#55` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-04-03T10:42:22.368Z
Learning: In PR `#67` for SakuraByteCore/codexmate, github-actions[bot] flagged that CodeRabbit was again introducing breaking changes. The maintainer's absolute constraint (reinforced across PRs `#55`, `#59`, `#67`) is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
🔇 Additional comments (7)
cli.js (2)
2821-2844: Good secret-redaction boundary.Sanitizing the OpenClaw auth-profile summary before returning it keeps
resolvedValueon the server side only. Nice hardening.Also applies to: 3055-3068
11048-11064: Nice fix on the usage API guard.Normalizing
paramsup front and forwarding the lower-casedsourcecloses both the missing-params crash and the" Codex "→alldrift.web-ui/modules/app.methods.openclaw-core.mjs (2)
327-473: Good auth-profile metadata propagation in quick-form hydration.The new
options.authProfilesByProviderflow andapiKeySource*mapping are consistent and keep the existing quick-form contract intact.
654-676: Secret/env display normalization looks solid.Formatting
SecretRef(...)andEnvRef(...)at render time improves clarity while preserving masking logic for plain sensitive strings.tests/e2e/test-openclaw.js (1)
27-30: Great regression coverage for secret redaction.The check that
resolvedValuestaysundefinedwhile keepingresolvedField/editableasserted is exactly the right safety net.tests/unit/web-ui-behavior-parity.test.mjs (2)
352-357: Nice catch on the non-HEAD data-key assertions.Filtering down to
unexpected*here keeps older parity baselines tolerant of approved drift without weakening the test.
407-412: Method-key parity now matches the data-key behavior.The non-HEAD path is checking only unexpected drift now, which is the right level of strictness for these allowlisted differences.
|
@coderabbitai If the current suggestion is not adopted, will it lead to disruptive changes? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me read the current state of the code after commit 🏁 Script executed: Length of output: 7192 🏁 Script executed: Length of output: 5804 🏁 Script executed: Length of output: 2313 Let me get the remaining critical details. 🏁 Script executed: Length of output: 6983
1. Session browser — tab-switch and freeze risksNo remaining risks. 2. Session usage summaries — params, exact message countsNo remaining risks.
3.
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me check the current state of the code after commit de4ac6a to see what remains outstanding. 🏁 Script executed: Length of output: 9065 Let me check the 🏁 Script executed: Length of output: 2965 🏁 Script executed: Length of output: 13781 🏁 Script executed: Length of output: 2630
1. Session browser behavior and session usage summaries ✅
2.
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 14531 🏁 Script executed: Length of output: 6773
1. Session browser behavior and usage summaries — ✅ clean
One low-risk observation: 2.
|
| Condition | Branch fires | value |
sourceOriginalValue |
|---|---|---|---|
resolvedValue present |
Never (stripped by sanitizer) | — | — |
editable === true && resolvedField |
✅ Now primary | '' |
'' |
display string present |
Fallback (read-only) | display string | '' |
The concern about spurious auth-profile updates does not materialize: applyOpenclawQuickToText (line 225) guards with if (apiKey && apiKey !== apiKeySourceOriginalValue). When apiKey is '' (user left field blank), the apiKey && short-circuit prevents any update from being staged regardless of sourceOriginalValue. ✓
The UI hint (apiKeyDisplayKind === 'auth-profile-value') still fires for the Branch 2 return (kind: 'auth-profile-value'), so the explanatory text about the auth-profile source is still shown correctly. ✓
UX note (not a bug): Users who previously saw their current key pre-filled (masked as ***) in the quick form will now see a blank input. This is a deliberate security tradeoff from the sanitization. The hint text already covers this case, but if you want to surface it more clearly, you could add a placeholder such as placeholder="（已配置，留空则不修改）" to the apiKey input when apiKeyDisplayKind === 'auth-profile-value'. This is cosmetic only and non-breaking.
🧠 Learnings used
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-31T12:37:51.023Z
Learning: In PR `#59` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-04-04T01:36:26.964Z
Learning: In PR `#68` for SakuraByteCore/codexmate, github-actions[bot] again flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint (reinforced across PRs `#55`, `#59`, `#67`, `#68`) is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-30T04:47:15.352Z
Learning: In PR `#55` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-04-03T10:42:22.368Z
Learning: In PR `#67` for SakuraByteCore/codexmate, github-actions[bot] flagged that CodeRabbit was again introducing breaking changes. The maintainer's absolute constraint (reinforced across PRs `#55`, `#59`, `#67`) is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-04-05T03:58:26.778Z
Learning: For SakuraByteCore/codexmate reviews, only provide non-breaking feedback (correctness, safety, quality). Avoid API, behavior, or refactor suggestions. This rule applies across PRs `#55`, `#59`, `#67`, and `#68`.
Learnt from: SurviveM
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-29T02:42:32.785Z
Learning: The repository maintainer SurviveM (GitHub: SurviveM, repo: SakuraByteCore/codexmate) prefers that CodeRabbit review suggestions do not introduce breaking changes. Reviews should focus on correctness, safety, and quality without recommending refactors or changes that would break existing APIs, interfaces, or behavior.
2 participants
Summary
Testing
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Documentation
Tests