feat: polish settings, reset workflow, Doctor, usage, and proxy streaming#180
Conversation
|
Warning Review limit reached
More reviews will be available in 17 minutes and 20 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds metadata-driven i18n and a single language-settings UI, internationalizes templates, introduces an OpenClaw accordion editor with validation, overhauls Responses↔Chat normalization and SSE streaming, adds release-changelog tooling and interactive reset CLI, plus tests, CSS, and a package version bump. ChangesMonolithic PR: i18n, OpenClaw, Proxy, Tests & Tools
Estimated code review effort: Possibly related PRs:
Suggested reviewers:
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web-ui/res/web-ui-render.precompiled.js`:
- Around line 132-137: The button's aria-label
(_ctx.t('settings.language.open')) overrides the visible text and hides the
current language from screen readers; change the aria-label to use the same
translated label as the visible text (use _ctx.t('settings.language.sideLabel',
{ language: _ctx.currentLanguageLabel() }) ), or remove the explicit aria-label
altogether so the accessible name comes from the button text; apply the same
change to the other identical occurrence that uses class
"language-settings-link" and onClick _ctx.openLanguageSettings.
🪄 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: 276a0a69-4196-4984-a84b-f33cb600a8fc
📒 Files selected for processing (9)
tests/unit/web-ui-behavior-parity.test.mjsweb-ui/modules/i18n.dict.mjsweb-ui/modules/i18n.mjsweb-ui/partials/index/layout-header.htmlweb-ui/partials/index/modal-webhook.htmlweb-ui/partials/index/panel-settings.htmlweb-ui/res/web-ui-render.precompiled.jsweb-ui/styles/layout-shell.cssweb-ui/styles/settings-panel.css
📜 Review details
🧰 Additional context used
🪛 HTMLHint (1.9.2)
web-ui/partials/index/modal-webhook.html
[error] 2-2: Doctype must be declared before any non-comment content.
(doctype-first)
🔇 Additional comments (9)
web-ui/modules/i18n.mjs (1)
5-33: LGTM!Also applies to: 52-59, 60-84, 93-93
web-ui/modules/i18n.dict.mjs (1)
157-158: LGTM!Also applies to: 916-980, 1249-1250, 1996-2060, 2330-2331, 3090-3150
tests/unit/web-ui-behavior-parity.test.mjs (1)
620-623: LGTM!web-ui/partials/index/layout-header.html (1)
96-101: LGTM!Also applies to: 270-270, 296-304, 309-314
web-ui/res/web-ui-render.precompiled.js (1)
366-366: LGTM!Also applies to: 397-399, 415-415, 3975-4004, 4055-4083, 4165-4165, 5849-5918
web-ui/partials/index/panel-settings.html (1)
43-63: LGTM!Also applies to: 100-114, 160-160
web-ui/partials/index/modal-webhook.html (1)
1-1: LGTM!Also applies to: 4-4, 7-7, 10-10, 15-15, 26-26, 36-36, 38-38
web-ui/styles/layout-shell.css (1)
668-711: LGTM!web-ui/styles/settings-panel.css (1)
417-419: LGTM!
|
Updated this PR for v0.0.39. Changes in the latest commit:
Validation actually run:
@coderabbitai re-review |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web-ui/modules/app.methods.runtime.mjs`:
- Around line 64-68: translateUiMessage currently relies on exact matches in
UI_MESSAGE_KEY_BY_TEXT so dynamic/prefixed toast texts like "导入成功..." or
"备份失败：..." are not localized; update translateUiMessage to detect and map such
cases by iterating UI_MESSAGE_KEY_BY_TEXT keys and finding the best prefix or
regex match (prefer longest matching key) against the incoming text, then call
context.t(foundKey) if a match is found, otherwise fall back to returning the
original text; reference translateUiMessage, UI_MESSAGE_KEY_BY_TEXT, and
context.t when locating where to implement this 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: 559b1a50-7314-4539-bca7-e2ea3647723c
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
package.jsonweb-ui/modules/app.computed.main-tabs.mjsweb-ui/modules/app.methods.providers.mjsweb-ui/modules/app.methods.runtime.mjsweb-ui/modules/config-mode.computed.mjsweb-ui/modules/i18n.dict.mjsweb-ui/partials/index/modals-basic.htmlweb-ui/partials/index/panel-config-codex.htmlweb-ui/partials/index/panel-trash.htmlweb-ui/res/web-ui-render.precompiled.js
✅ Files skipped from review due to trivial changes (3)
- web-ui/modules/app.methods.providers.mjs
- package.json
- web-ui/res/web-ui-render.precompiled.js
📜 Review details
🧰 Additional context used
🪛 HTMLHint (1.9.2)
web-ui/partials/index/panel-trash.html
[error] 2-2: Doctype must be declared before any non-comment content.
(doctype-first)
web-ui/partials/index/panel-config-codex.html
[error] 129-129: Special characters must be escaped : [ > ].
(spec-char-escape)
🔇 Additional comments (6)
web-ui/partials/index/panel-trash.html (1)
18-18: LGTM!Also applies to: 34-34, 41-42, 49-49, 66-67, 69-70, 77-77
web-ui/partials/index/modals-basic.html (1)
175-175: LGTM!Also applies to: 180-183, 185-186, 190-191
web-ui/modules/app.computed.main-tabs.mjs (1)
46-48: LGTM!Also applies to: 50-84, 88-137, 151-152, 163-164, 175-176, 206-207, 209-210
web-ui/modules/config-mode.computed.mjs (1)
38-39: LGTM!Also applies to: 41-42, 47-48
web-ui/modules/i18n.dict.mjs (1)
61-65: LGTM!Also applies to: 70-123, 231-231, 774-780, 912-941, 1048-1054, 1063-1063, 1254-1258, 1263-1316, 1424-1424, 1955-1960, 2093-2122, 2229-2235, 2244-2244, 2436-2440, 2445-2498, 2606-2606, 3150-3155, 3288-3317, 3420-3427, 3435-3435
web-ui/partials/index/panel-config-codex.html (1)
125-125: LGTM!Also applies to: 129-129, 143-143
|
Fixed the CodeRabbit actionable comment in a follow-up commit. Latest commit:
What changed:
Validation actually run after the fix:
@coderabbitai re-review |
|
(ᵔᴥᵔ)🐇 ✅ Actions performedFull review triggered. |
awsl233777
changed the title
awsl233777
force-pushed
the
feat/settings-language-selector
branch
from
7249ba4 to
97370b5
Compare
awsl233777
changed the title
|
Added a follow-up fix for the built-in Codex proxy streaming path. Latest commit:
What changed:
Validation actually run:
@coderabbitai review |
|
(ᵔᴥᵔ)🐇 ✅ Actions performedReview triggered.
|
awsl233777
force-pushed
the
feat/settings-language-selector
branch
from
381dc38 to
c35d11e
Compare
awsl233777
changed the title
Localize remaining Settings/Webhook/Trash/provider/toast UI labels, keep dynamic toast translation fallback behavior, refresh the precompiled Web UI render bundle, bump package metadata to 0.0.39, and retry built-in proxy streaming aborts before any client-visible SSE output.
awsl233777
force-pushed
the
feat/settings-language-selector
branch
from
c35d11e to
ab4d7cd
Compare
|
Updated the built-in Codex proxy stream hardening after observing long-running Codex work still stopping around the short request timeout window. Latest head:
Additional fix:
Validation actually run:
@coderabbitai review |
|
(ᵔᴥᵔ)🐇 ✅ Actions performedReview triggered.
|
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/builtin-proxy.js (1)
946-967:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDelay SSE commitment in the non-stream JSON fallback path too.
This branch still sends
200 text/event-streambefore the upstream body completes. If that JSON response aborts mid-transfer,handleAbort()seesres.headersSentand turns it into a visibleresponse.failedinstead of returning{ ok: false }for the transient retry path. That leaves one pre-output abort case un-hardened.Suggested fix
if (!/text\/event-stream/i.test(contentType)) { - res.writeHead(200, { - 'Content-Type': 'text/event-stream; charset=utf-8', - 'Cache-Control': 'no-cache', - 'Connection': 'keep-alive', - 'X-Accel-Buffering': 'no' - }); upstreamRes.on('data', (chunk) => chunk && chunks.push(chunk)); upstreamRes.on('end', () => { const text = chunks.length ? Buffer.concat(chunks).toString('utf-8') : ''; const parsedJson = parseJsonOrError(text); + res.writeHead(200, { + 'Content-Type': 'text/event-stream; charset=utf-8', + 'Cache-Control': 'no-cache', + 'Connection': 'keep-alive', + 'X-Accel-Buffering': 'no' + }); if (parsedJson.error) { writeSse(res, 'response.failed', { type: 'response.failed', error: `invalid upstream response: ${parsedJson.error}` }); writeSse(res, 'done', '[DONE]'); res.end(); finish({ ok: true });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/builtin-proxy.js` around lines 946 - 967, The branch that handles non-stream JSON currently calls res.writeHead('Content-Type': 'text/event-stream'...) immediately, causing headersSent to be true before upstreamRes completes; move the res.writeHead call(s) into the upstreamRes.on('end') handler (and into the error path) so headers are only sent after parsing via parseJsonOrError succeeds or fails, then call writeSse/sendResponsesSse and res.end, and finally finish({ok:true}) or finish({ok:false}) as appropriate; update references in this block (res.writeHead, upstreamRes.on('end', ...), parseJsonOrError, writeSse, sendResponsesSse, finish) so aborts before end do not see headersSent and handleAbort can return {ok:false}.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web-ui/partials/index/panel-trash.html`:
- Around line 66-69: The restore and purge icon-only buttons (handlers
restoreSessionTrash and purgeSessionTrash, using
sessionTrashRestoring/sessionTrashPurging and getSessionTrashActionKey) lack
accessible names for screen readers; add an explicit aria-label (or :aria-label
bound) to each button that mirrors the computed title text (e.g. the same
conditional used in :title:
sessionTrashRestoring[getSessionTrashActionKey(item)] ?
t('settings.trash.restoring') : t('settings.trash.restore') for the restore
button and sessionTrashPurging[...] ? t('settings.trash.purging') :
t('settings.trash.purge') for the purge button) so assistive tech gets a
reliable label while keeping the existing title and disabled logic.
---
Outside diff comments:
In `@cli/builtin-proxy.js`:
- Around line 946-967: The branch that handles non-stream JSON currently calls
res.writeHead('Content-Type': 'text/event-stream'...) immediately, causing
headersSent to be true before upstreamRes completes; move the res.writeHead
call(s) into the upstreamRes.on('end') handler (and into the error path) so
headers are only sent after parsing via parseJsonOrError succeeds or fails, then
call writeSse/sendResponsesSse and res.end, and finally finish({ok:true}) or
finish({ok:false}) as appropriate; update references in this block
(res.writeHead, upstreamRes.on('end', ...), parseJsonOrError, writeSse,
sendResponsesSse, finish) so aborts before end do not see headersSent and
handleAbort can return {ok:false}.
🪄 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: 1e739f65-bd45-42b3-86ce-f36326cdb1bd
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
cli/builtin-proxy.jspackage.jsontests/unit/builtin-proxy-responses-shim.test.mjsweb-ui/modules/app.computed.main-tabs.mjsweb-ui/modules/app.methods.providers.mjsweb-ui/modules/app.methods.runtime.mjsweb-ui/modules/config-mode.computed.mjsweb-ui/modules/i18n.dict.mjsweb-ui/partials/index/modals-basic.htmlweb-ui/partials/index/panel-config-codex.htmlweb-ui/partials/index/panel-trash.htmlweb-ui/res/web-ui-render.precompiled.js
✅ Files skipped from review due to trivial changes (1)
- package.json
🚧 Files skipped from review as they are similar to previous changes (6)
- web-ui/modules/app.methods.providers.mjs
- web-ui/partials/index/modals-basic.html
- web-ui/modules/app.methods.runtime.mjs
- web-ui/modules/app.computed.main-tabs.mjs
- web-ui/modules/i18n.dict.mjs
- web-ui/res/web-ui-render.precompiled.js
📜 Review details
🧰 Additional context used
🪛 HTMLHint (1.9.2)
web-ui/partials/index/panel-config-codex.html
[error] 129-129: Special characters must be escaped : [ > ].
(spec-char-escape)
web-ui/partials/index/panel-trash.html
[error] 2-2: Doctype must be declared before any non-comment content.
(doctype-first)
|
@coderabbitai Could you please do a focused high-severity pass on the current PR head? Please list only:
If there are no P0/P1 blockers, please say that explicitly and separate lower-priority suggestions from merge blockers. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Read the proxy file for retry logic, timeout, and SSE state flags
cat cli/builtin-proxy.jsLength of output: 76198 🏁 Script executed: #!/bin/bash
# Read the new proxy shim test
cat tests/unit/builtin-proxy-responses-shim.test.mjsLength of output: 32804 🏁 Script executed: #!/bin/bash
# Read the runtime methods for translateUiMessage
cat web-ui/modules/app.methods.runtime.mjsLength of output: 16533 🏁 Script executed: #!/bin/bash
# Read the i18n module for applyDocumentLanguage and new methods
cat web-ui/modules/i18n.mjsLength of output: 4265
✅ No P0/P1 blockersThe PR is safe to merge. All critical invariants hold:
P2 — Concrete improvement suggestions for proxy streamingThese are not merge blockers but are worth tracking. 1. Infinite reschedule for accepted-stream timeouts (resource leak) Once req.setTimeout(timeoutMs, () => {
if (streamAccepted) {
req.setTimeout(STREAM_IDLE_TIMEOUT_MS); // fires again → reschedules again → forever
return;
}
// …destroy path is never reached
});Suggested fix — cap the reschedule or honour a maximum idle count: let idleExtensions = 0;
const MAX_IDLE_EXTENSIONS = 3; // ~30 min total after stream accepted
req.setTimeout(timeoutMs, () => {
if (streamAccepted && idleExtensions < MAX_IDLE_EXTENSIONS) {
idleExtensions += 1;
req.setTimeout(STREAM_IDLE_TIMEOUT_MS);
return;
}
try { req.destroy(new Error('stream idle timeout')); } catch (_) {}
finish({ ok: false, error: 'stream idle timeout' });
});2. Client disconnect doesn't abort the upstream request The Suggested addition in res.on('close', () => {
stopChatStreamHeartbeat(state);
if (!state.finished) {
try { /* ref to upstream req */ upstreamReq.destroy(); } catch (_) {}
}
});This requires threading P2 — i18n compatibility note
// current
return getAvailableLanguages().some(...) ? normalized : 'zh';
// more robust
const available = getAvailableLanguages();
const defaultCode = available.length > 0 ? available[0].code : 'zh';
return available.some(...) ? normalized : defaultCode;Accessibility — trash actions ✅The trash panel |
|
P2 follow-up fixed in Changes:
Validation actually run:
@coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check idle extension cap in builtin-proxy.js
rg -n "idleExtensions\|MAX_IDLE\|idle.*extension\|extension.*idle\|STREAM_IDLE\|streamAccepted" cli/builtin-proxy.js -A2 -B2
echo "---CLIENT DISCONNECT DESTROY---"
# Check upstream destroy on client disconnect
rg -n "destroy\|upstreamReq\|res\.on.*close\|close.*destroy" cli/builtin-proxy.js -A3 -B1
echo "---LANGUAGE FALLBACK---"
# Check language fallback in i18n.mjs
rg -n "normalizeLang\|getAvailableLanguages\|fallback\|defaultCode" web-ui/modules/i18n.mjs -A3 -B1Length of output: 828 🏁 Script executed: #!/bin/bash
# Find and verify the proxy file exists
fd "builtin-proxy.js" --type f
echo "---"
# Check i18n.mjs
fd "i18n.mjs" web-ui --type f
echo "---PROXY IDLE SECTION---"
grep -n "STREAM_IDLE\|idleExtension\|MAX_IDLE\|streamAccepted\|setTimeout" cli/builtin-proxy.js | head -40
echo "---DESTROY ON CLOSE---"
grep -n "destroy\|\.on('close\|\.on(\"close" cli/builtin-proxy.js | head -30
echo "---LANGUAGE FALLBACK---"
grep -n "normalizeLang\|getAvailableLanguages\|defaultCode\|available\[0\]" web-ui/modules/i18n.mjs | head -20Length of output: 3124
540/540 unit tests + 13/13 proxy shim tests passing on top of these is solid coverage. Kicking off the review now. (ᵔᴥᵔ)🐇 ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
web-ui/res/web-ui-render.precompiled.js (1)
132-137:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep the current language in the button accessible name.
At Line 135 and Line 425,
aria-labeloverrides visible text, so assistive tech won’t read the current language that is shown insettings.language.sideLabel(...).Suggested fix
- "aria-label": _ctx.t('settings.language.open'), + "aria-label": _ctx.t('settings.language.sideLabel', { language: _ctx.currentLanguageLabel() }),Also applies to: 422-427
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-ui/res/web-ui-render.precompiled.js` around lines 132 - 137, The button's aria-label currently uses _ctx.t('settings.language.open') which overrides the visible text and hides the current language; update the aria-label on the _createElementVNode call(s) that render the language settings button (the instance using _ctx.openLanguageSettings and displaying _ctx.t('settings.language.sideLabel', { language: _ctx.currentLanguageLabel() })) so it includes the current language—e.g., build the label by combining the open text with _ctx.currentLanguageLabel() or the same sideLabel translation—so assistive tech announces both the action and the current language.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@web-ui/res/web-ui-render.precompiled.js`:
- Around line 132-137: The button's aria-label currently uses
_ctx.t('settings.language.open') which overrides the visible text and hides the
current language; update the aria-label on the _createElementVNode call(s) that
render the language settings button (the instance using
_ctx.openLanguageSettings and displaying _ctx.t('settings.language.sideLabel', {
language: _ctx.currentLanguageLabel() })) so it includes the current
language—e.g., build the label by combining the open text with
_ctx.currentLanguageLabel() or the same sideLabel translation—so assistive tech
announces both the action and the current language.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9927b629-47dc-4ea4-b7a2-e628f244615a
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
cli/builtin-proxy.jspackage.jsontests/unit/builtin-proxy-responses-shim.test.mjsweb-ui/modules/app.computed.main-tabs.mjsweb-ui/modules/app.methods.providers.mjsweb-ui/modules/app.methods.runtime.mjsweb-ui/modules/config-mode.computed.mjsweb-ui/modules/i18n.dict.mjsweb-ui/modules/i18n.mjsweb-ui/partials/index/modals-basic.htmlweb-ui/partials/index/panel-config-codex.htmlweb-ui/partials/index/panel-trash.htmlweb-ui/res/web-ui-render.precompiled.js
✅ Files skipped from review due to trivial changes (1)
- web-ui/modules/app.methods.providers.mjs
🚧 Files skipped from review as they are similar to previous changes (6)
- package.json
- web-ui/modules/app.methods.runtime.mjs
- web-ui/partials/index/modals-basic.html
- web-ui/modules/config-mode.computed.mjs
- web-ui/modules/app.computed.main-tabs.mjs
- web-ui/modules/i18n.dict.mjs
📜 Review details
🧰 Additional context used
🪛 HTMLHint (1.9.2)
web-ui/partials/index/panel-config-codex.html
[error] 129-129: Special characters must be escaped : [ > ].
(spec-char-escape)
web-ui/partials/index/panel-trash.html
[error] 2-2: Doctype must be declared before any non-comment content.
(doctype-first)
🔇 Additional comments (29)
web-ui/modules/i18n.mjs (7)
16-19: LGTM!
21-24: LGTM!
26-31: LGTM!
33-40: LGTM!
53-60: LGTM!
61-91: LGTM!
92-101: LGTM!cli/builtin-proxy.js (13)
137-137: LGTM!
142-143: LGTM!
836-866: LGTM!
891-934: LGTM!
948-952: LGTM!
955-979: LGTM!
995-1018: LGTM!
1020-1039: LGTM!
1042-1060: LGTM!
1062-1073: LGTM!
1080-1084: LGTM!
1382-1387: LGTM!
1540-1551: LGTM!tests/unit/builtin-proxy-responses-shim.test.mjs (6)
110-127: LGTM!
564-621: LGTM!
623-683: LGTM!
685-730: LGTM!
732-791: LGTM!
793-844: LGTM!web-ui/res/web-ui-render.precompiled.js (1)
1267-1305: LGTM!Also applies to: 1349-1377, 4195-4372, 5665-5742
web-ui/partials/index/panel-config-codex.html (1)
125-125: LGTM!Also applies to: 129-129, 143-143
web-ui/partials/index/panel-trash.html (1)
17-18: LGTM!Also applies to: 24-24, 33-34, 41-42, 45-45, 49-49, 66-66, 69-69, 77-77
|
Updated the built-in proxy retry behavior after re-checking the stream retry boundary:
Validation run locally:
@coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "---IDLE TIMEOUT / EXTENSION CAP---"
grep -n "STREAM_IDLE\|idleExtension\|MAX_IDLE\|streamAccepted\|setTimeout\|maxStreamIdle" cli/builtin-proxy.js | head -40
echo "---CLIENT DISCONNECT / DESTROY---"
grep -n "destroy\|\.on('close\|\.on(\"close\|client aborted\|upstreamReq" cli/builtin-proxy.js | head -30
echo "---RETRY LOGIC---"
grep -n "TRANSIENT_RETRY\|retryTransient\|retry\|started\|finished" cli/builtin-proxy.js | head -40
echo "---STATE FLAGS---"
grep -n "state\.started\|state\.finished\|streamState\|closeListenerAttached" cli/builtin-proxy.js | head -30
echo "---TEST COUNT---"
grep -n "test\|suite\|describe\|it(" tests/unit/builtin-proxy-responses-shim.test.mjs | grep -i "^[0-9]" | wc -l
grep -c "^test\|^ test\|suite\|describe" tests/unit/builtin-proxy-responses-shim.test.mjsLength of output: 5435
No P0/P1 blockers.
On the idle-cap removal: The accepted stream timeout is now fully disabled via Test count drop (13→12 / 540→539): Expected — the idle-extension-cap regression test was removed along with the feature. The remaining 12 shim tests cover the critical paths. Kicking off the review now. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cli/builtin-proxy.js (1)
1071-1073: 💤 Low valueConsider explicit heartbeat cleanup on final retry failure.
When all retries fail and the caller's error handler writes
response.faileddirectly tores, the heartbeat interval continues running until its next tick (up to 15 seconds later) when it noticesres.writableEnded. The timer is unref'd so it won't block process exit, but for cleaner resource management, consider returningstreamStatein the result so the caller can explicitly stop the heartbeat.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/builtin-proxy.js` around lines 1071 - 1073, The current retry loop creates a heartbeat via createChatStreamResponsesSseState and calls retryTransientRequest(() => streamChatCompletionsAsResponsesSse(...)) but on final failure the caller only receives the error and cannot stop the heartbeat; change the call site and the underlying functions so that when retryTransientRequest returns a failure it includes the streamState (e.g., return { error, streamState } or similar), and update streamChatCompletionsAsResponsesSse and retryTransientRequest to propagate streamState on both success and failure so the caller can explicitly stop the heartbeat (use the existing streamState variable created from createChatStreamResponsesSseState and ensure callers can call its stop/cleanup method).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cli/builtin-proxy.js`:
- Around line 1071-1073: The current retry loop creates a heartbeat via
createChatStreamResponsesSseState and calls retryTransientRequest(() =>
streamChatCompletionsAsResponsesSse(...)) but on final failure the caller only
receives the error and cannot stop the heartbeat; change the call site and the
underlying functions so that when retryTransientRequest returns a failure it
includes the streamState (e.g., return { error, streamState } or similar), and
update streamChatCompletionsAsResponsesSse and retryTransientRequest to
propagate streamState on both success and failure so the caller can explicitly
stop the heartbeat (use the existing streamState variable created from
createChatStreamResponsesSseState and ensure callers can call its stop/cleanup
method).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1b64cc2a-f3fd-4067-bffb-2932c8f73c94
📒 Files selected for processing (2)
cli/builtin-proxy.jstests/unit/builtin-proxy-responses-shim.test.mjs
📜 Review details
🔇 Additional comments (7)
tests/unit/builtin-proxy-responses-shim.test.mjs (1)
113-113: LGTM!Also applies to: 558-616, 680-725
cli/builtin-proxy.js (6)
137-141: LGTM!Also applies to: 161-161
715-716: LGTM!Also applies to: 801-801
836-866: LGTM!
876-897: LGTM!
916-916: LGTM!Also applies to: 1010-1014
947-968: LGTM!
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web-ui/styles/openclaw-structured.css`:
- Around line 128-131: The keyframe name accordionSlide violates the kebab-case
keyframes-name-pattern; rename the keyframe and its usage together (e.g.,
accordion-slide) by updating the `@keyframes` declaration name and any references
to it in the stylesheet (for example the animation: property that currently uses
accordionSlide) so both match the new kebab-case identifier.
🪄 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: e357d674-24b7-46c3-8d3c-89c34436c727
📒 Files selected for processing (8)
tests/unit/web-ui-behavior-parity.test.mjsweb-ui/app.jsweb-ui/modules/app.methods.openclaw-editing.mjsweb-ui/partials/index/modal-openclaw-config.htmlweb-ui/partials/index/panel-config-openclaw.htmlweb-ui/res/web-ui-render.precompiled.jsweb-ui/styles/controls-forms.cssweb-ui/styles/openclaw-structured.css
✅ Files skipped from review due to trivial changes (1)
- web-ui/styles/controls-forms.css
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/unit/web-ui-behavior-parity.test.mjs
- web-ui/partials/index/modal-openclaw-config.html
- web-ui/modules/app.methods.openclaw-editing.mjs
- web-ui/partials/index/panel-config-openclaw.html
- web-ui/app.js
📜 Review details
🧰 Additional context used
🪛 Stylelint (17.12.0)
web-ui/styles/openclaw-structured.css
[error] 131-131: Expected keyframe name "accordionSlide" to be kebab-case (keyframes-name-pattern)
(keyframes-name-pattern)
🔇 Additional comments (1)
web-ui/styles/openclaw-structured.css (1)
168-176: Duplicate.form-hint.hint-errorrule remains.This selector is still defined twice with identical declarations; keep only one block.
| animation: accordionSlide 0.2s ease; | ||
| } | ||
|
|
||
| @keyframes accordionSlide { |
There was a problem hiding this comment.
Fix keyframe naming to satisfy stylelint rule.
accordionSlide violates the configured keyframes-name-pattern (kebab-case). Rename the keyframe and its usage together.
Proposed fix
.accordion-content {
padding: 0 16px 16px;
- animation: accordionSlide 0.2s ease;
+ animation: accordion-slide 0.2s ease;
}
-@keyframes accordionSlide {
+@keyframes accordion-slide {
from {
opacity: 0;
transform: translateY(-8px);
}📝 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.
| animation: accordionSlide 0.2s ease; | |
| } | |
| @keyframes accordionSlide { | |
| animation: accordion-slide 0.2s ease; | |
| } | |
| `@keyframes` accordion-slide { |
🧰 Tools
🪛 Stylelint (17.12.0)
[error] 131-131: Expected keyframe name "accordionSlide" to be kebab-case (keyframes-name-pattern)
(keyframes-name-pattern)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web-ui/styles/openclaw-structured.css` around lines 128 - 131, The keyframe
name accordionSlide violates the kebab-case keyframes-name-pattern; rename the
keyframe and its usage together (e.g., accordion-slide) by updating the
`@keyframes` declaration name and any references to it in the stylesheet (for
example the animation: property that currently uses accordionSlide) so both
match the new kebab-case identifier.
- Remove '提示：应用后写入 ~/.openclaw/openclaw.json' hint text - Keep workspace card functionality intact
ymkiux
force-pushed
the
feat/settings-language-selector
branch
from
05beed7 to
f5245bc
Compare
- Reduce layout gap from 16px to 12px - Reduce tools grid gap from 12px to 8px - Reduce button padding from 16px to 12px - Reduce button content gap from 12px to 10px
- Remove '选择常用配置，或进入编辑器维护完整 JSON5' hint
There was a problem hiding this comment.
♻️ Duplicate comments (2)
web-ui/styles/openclaw-structured.css (2)
126-132:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRename keyframe to kebab-case to satisfy lint.
Line 128 and Line 131 still use
accordionSlide, which violates the configured keyframe naming rule and can fail stylelint.Proposed minimal fix
.accordion-content { padding: 0 16px 16px; - animation: accordionSlide 0.2s ease; + animation: accordion-slide 0.2s ease; } -@keyframes accordionSlide { +@keyframes accordion-slide { from { opacity: 0; transform: translateY(-8px); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-ui/styles/openclaw-structured.css` around lines 126 - 132, The keyframe name accordionSlide violates the kebab-case lint rule; rename the keyframe to kebab-case (e.g., accordion-slide) and update all references: change the `@keyframes` declaration name and the .accordion-content animation property value (animation: accordion-slide 0.2s ease;) so the `@keyframes` rule and the animation usage (and any from/to blocks) match the new kebab-case identifier.
168-176:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove duplicated
.form-hint.hint-errorrule.The same selector is declared twice with identical declarations (Line 169-171 and Line 174-176), which is redundant and noisy.
Proposed minimal fix
/* Form hint error state */ .form-hint.hint-error { color: var(--color-error); } - -/* Form hint error state */ -.form-hint.hint-error { - color: var(--color-error); -}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-ui/styles/openclaw-structured.css` around lines 168 - 176, Duplicate CSS rule .form-hint.hint-error is declared twice; remove the redundant block so only a single .form-hint.hint-error { color: var(--color-error); } remains (locate the two identical selectors .form-hint.hint-error in openclaw-structured.css and delete the second/duplicate declaration).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@web-ui/styles/openclaw-structured.css`:
- Around line 126-132: The keyframe name accordionSlide violates the kebab-case
lint rule; rename the keyframe to kebab-case (e.g., accordion-slide) and update
all references: change the `@keyframes` declaration name and the
.accordion-content animation property value (animation: accordion-slide 0.2s
ease;) so the `@keyframes` rule and the animation usage (and any from/to blocks)
match the new kebab-case identifier.
- Around line 168-176: Duplicate CSS rule .form-hint.hint-error is declared
twice; remove the redundant block so only a single .form-hint.hint-error {
color: var(--color-error); } remains (locate the two identical selectors
.form-hint.hint-error in openclaw-structured.css and delete the second/duplicate
declaration).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c822b6c7-345a-401a-a994-d090694dcad9
📒 Files selected for processing (3)
web-ui/partials/index/panel-config-openclaw.htmlweb-ui/res/web-ui-render.precompiled.jsweb-ui/styles/openclaw-structured.css
💤 Files with no reviewable changes (1)
- web-ui/partials/index/panel-config-openclaw.html
📜 Review details
🔇 Additional comments (1)
web-ui/styles/openclaw-structured.css (1)
357-383: LGTM!
- Change tools grid from 2-column to single column flex - AGENTS.md button and workspace file now stack vertically
- Change card layout from horizontal to vertical - Icon/content and pill/actions now stack top to bottom
- Hide card icon (first letter circle) from config cards - Remove left visual anchor for cleaner look
- Remove 'OpenClaw 配置' heading from config card
- Remove white-space/overflow constraints on title and subtitle - Allow card content to use full available width - Card text now wraps naturally instead of truncating
- Reduce workspace card padding from 16px to 12px/14px - Add width: 100% to input group and form input - Add width: 100% to tool button - Elements now use full available width
- Add explicit padding to outer workspace/config sections - Remove duplicate styles from nested workspace card - Distinguish outer section card from nested card with specific selectors - Ensure x-position alignment between workspace elements and config cards
|
Updated this PR with a metapi-style hardening pass for the built-in What changed:
Validated:
@coderabbitai please re-review the latest head, focusing on the Responses/chat conversion contract and any compatibility regressions in provider payload forwarding. |
|
To recap the outcome of that earlier review and everything that followed: That review result (no P0/P1 blockers):
Since then, the PR has accumulated many more verified fixes:
Current state: 556/556 tests passing, no P0/P1 blockers. Safe to merge. ✅ Actions performedReview triggered.
|
|
Follow-up UI polish after visual review:
Validation actually run after this follow-up:
@coderabbitai re-review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== openclaw-structured.css current state ==="
cat web-ui/styles/openclaw-structured.css
echo ""
echo "=== panel-config-openclaw.html - aria-label on card actions ==="
grep -n "aria-label\|applyAria\|editAria\|deleteAria\|btn-mini\|openclaw-card-actions" web-ui/partials/index/panel-config-openclaw.html | head -30
echo ""
echo "=== precompiled bundle - openclaw section layout classes ==="
grep -n "openclaw-workspace-card\|openclaw-tools-grid\|openclaw-tool-btn\|openclaw-action-grid\|openclaw-hero\|openclaw-overview\|openclaw-primary\|openclaw-configs" web-ui/res/web-ui-render.precompiled.js | head -40
echo ""
echo "=== responsive.css - openclaw breakpoints ==="
grep -n "openclaw\|980px\|820px" web-ui/styles/responsive.css | head -30
echo ""
echo "=== Check for any conflict markers ==="
rg -n "<<<<<<< |>>>>>>> |=======" web-ui/styles/ 2>/dev/null | head -10Length of output: 15738
Still outstanding: P2 —
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web-ui/styles/openclaw-structured.css (1)
558-564:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClamp won’t apply with visible overflow.
-webkit-line-clamp: 2at Line 562 won’t reliably truncate while overflow is set to visible in the earlier.card-subtitlerule (Line 540). Set overflow to hidden in the clamped rule.Proposed fix
.openclaw-card-list .card-subtitle { white-space: normal; display: -webkit-box; -webkit-box-orient: vertical; -webkit-line-clamp: 2; + overflow: hidden; overflow-wrap: anywhere; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-ui/styles/openclaw-structured.css` around lines 558 - 564, The .openclaw-card-list .card-subtitle rule uses -webkit-line-clamp: 2 but the earlier .card-subtitle rule sets overflow: visible, preventing clamping; update the clamped rule (selector .openclaw-card-list .card-subtitle) to set overflow: hidden (and optionally add text-overflow: ellipsis; white-space: normal stays) so -webkit-line-clamp can apply, or remove/override the conflicting overflow: visible in the base .card-subtitle rule.
♻️ Duplicate comments (1)
web-ui/styles/openclaw-structured.css (1)
128-131:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRename keyframe to kebab-case to satisfy lint.
Line 128 and Line 131 still use
accordionSlide; this continues to violate the configured keyframe naming rule and can fail stylelint.Proposed fix
.accordion-content { padding: 0 16px 16px; - animation: accordionSlide 0.2s ease; + animation: accordion-slide 0.2s ease; } -@keyframes accordionSlide { +@keyframes accordion-slide {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-ui/styles/openclaw-structured.css` around lines 128 - 131, The keyframe name accordionSlide violates the kebab-case lint rule; rename the `@keyframes` declaration from accordionSlide to a kebab-case name (e.g., accordion-slide) and update the animation property usage (animation: accordionSlide 0.2s ease;) to use the new kebab-case name (animation: accordion-slide 0.2s ease;), and search for any other occurrences of the symbol accordionSlide in the stylesheet to replace them consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@web-ui/styles/openclaw-structured.css`:
- Around line 558-564: The .openclaw-card-list .card-subtitle rule uses
-webkit-line-clamp: 2 but the earlier .card-subtitle rule sets overflow:
visible, preventing clamping; update the clamped rule (selector
.openclaw-card-list .card-subtitle) to set overflow: hidden (and optionally add
text-overflow: ellipsis; white-space: normal stays) so -webkit-line-clamp can
apply, or remove/override the conflicting overflow: visible in the base
.card-subtitle rule.
---
Duplicate comments:
In `@web-ui/styles/openclaw-structured.css`:
- Around line 128-131: The keyframe name accordionSlide violates the kebab-case
lint rule; rename the `@keyframes` declaration from accordionSlide to a kebab-case
name (e.g., accordion-slide) and update the animation property usage (animation:
accordionSlide 0.2s ease;) to use the new kebab-case name (animation:
accordion-slide 0.2s ease;), and search for any other occurrences of the symbol
accordionSlide in the stylesheet to replace them consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 752da7fa-f233-41fb-ba2f-f5ddcd52e9e9
📒 Files selected for processing (1)
web-ui/styles/openclaw-structured.css
📜 Review details
🔇 Additional comments (1)
web-ui/styles/openclaw-structured.css (1)
169-176: Remove duplicated.form-hint.hint-errordeclaration.Line 169-171 and Line 174-176 define the same selector/property pair; keep only one block.
|
@coderabbitai Please do one focused follow-up pass on the latest head and answer with:
Please avoid repeating already-resolved or duplicate findings unless they are still valid on the current head. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== applyAria on card actions ==="
grep -n "applyAria\|aria-label.*apply\|apply.*aria-label\|btn-mini\|openclaw-card-actions" web-ui/partials/index/panel-config-openclaw.html | head -30
echo ""
echo "=== Precompiled bundle - openclaw card apply aria-label ==="
grep -n "applyAria\|openclawAction\|openclaw.*action.*apply" web-ui/res/web-ui-render.precompiled.js | head -20
echo ""
echo "=== openclaw.action.applyAria in i18n dict ==="
grep -n "applyAria" web-ui/modules/i18n.dict.mjs | head -10
echo ""
echo "=== config-tabs-ui.test.mjs - openclaw apply aria-label assertion ==="
grep -n "applyAria\|aria-label.*apply\|openclaw.*action" tests/unit/config-tabs-ui.test.mjs | head -20
echo ""
echo "=== form-hint.hint-error duplicate in openclaw-structured.css ==="
grep -n "hint-error\|hint_error" web-ui/styles/openclaw-structured.css
echo ""
echo "=== Conflict markers check ==="
rg -n "<<<<<<< |>>>>>>> |=======" cli/ web-ui/ 2>/dev/null | grep -v ".git" | grep -v Binary | head -15
echo ""
echo "=== Proxy state machine guards still intact ==="
grep -n "state\.started\|state\.finished\|state\.upstreamReq\|state\.outputStarted" cli/builtin-proxy.js | head -15
echo ""
echo "=== Credential visibility resets ==="
grep -n "showAddProviderKey\|showAddClaudeConfigKey\|showEditProviderKey\|showEditClaudeConfigKey" web-ui/app.js | head -15
echo ""
echo "=== Test counts ==="
grep -c "^test(" tests/unit/builtin-proxy-responses-shim.test.mjs 2>/dev/null
grep -c "^test(" tests/unit/openai-bridge-upstream-responses.test.mjs 2>/dev/null
grep -c "^test(" tests/unit/reset-main.test.mjs 2>/dev/null
echo ""
echo "=== Doctor panel - navigate guard and export availability ==="
grep -n "navigate\|action\.target\|healthCheckResult\.report\|export" web-ui/partials/index/panel-dashboard.html | head -30
echo ""
echo "=== hardcoded Cancel string in OpenClaw modal footer ==="
grep -n "取消\|Cancel\|cancel" web-ui/partials/index/modal-openclaw-config.html | head -10
echo ""
echo "=== resolveResetTargetPrNumber - destructive reset gate ==="
grep -n "buildResetPlan\|executeResetPlan\|resolveResetTarget\|hasResetTargetArg\|main\b" tools/dev/reset-main.js | head -30Length of output: 8604
1. ✅ No P0/P1 blockersAll critical invariants hold on the current head:
2. Previously open P2 — now resolved ✅
3. Non-blocking improvement suggestionsP2 — The test file asserts assert.match(html, /openclaw\.action\.applyAria/);P3 — Hardcoded The OpenClaw modal edit footer Cancel button still uses a hardcoded Chinese string: <button class="btn btn-cancel" `@click`="closeOpenclawConfigModal" ...>取消</button>
P3 — The selector appears twice with identical rules. Safe to deduplicate when convenient. P3 —
No P0/P1 blockers remain. Safe to merge. (ᵔᴥᵔ)🎉 |
2 participants
Summary
npm run resettarget-selection flow: explicit PR numbers still reset directly to that PR snapshot, while no-arg interactive reset asks for a target where Enter selectsorigin/mainand a numeric input selects the matching PR.apply_patch.tool_calls:custom_tool_call,local_shell_call, and ordinaryfunction_callno longer collapse into one generic shape.Validation
NODE_PATH=/home/moltbot/clawd-wechat/codexmate/node_modules npm run test:unit→All 563 tests passed.NODE_PATH=/home/moltbot/clawd-wechat/codexmate/node_modules npm run lint→Lint passed for 204 file(s).NODE_PATH=/home/moltbot/clawd-wechat/codexmate/node_modules npm run test:e2e→ passed.已配置status visible.web-ui/res/web-ui-render.precompiled.js.NODE_PATH=/home/moltbot/clawd-wechat/codexmate/node_modules node tools/dev/reset-main.jswith no args in non-interactive mode → exits1before reset/clean and prints the reset-target-required error.Notes
REVIEW_REQUIRED) if checks stay green.Summary by CodeRabbit
New Features
Improvements
Tests
Chores