feat: modularize web ui and harden local workflows#71
feat: modularize web ui and harden local workflows#71ymkiux merged 6 commits intoSakuraByteCore:mainfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 1 minutes and 13 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a new "device" settings tab to the web UI, moves the config reset UI into that device panel, updates tab normalization to preserve "device", adjusts modal and responsive CSS (including an Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/config-tabs-ui.test.mjs`:
- Line 431: The mobile-size regex is too broad—it's matching any
.session-item-copy rule and can miss a specific pin override; update the
assertion to target the pin selector by changing the pattern to look for
.session-item-copy.session-item-pin (or the exact selector order used in the
stylesheet) within the `@media` (max-width: 540px) block and assert the
width/height/min-width/min-height values for that selector so the test verifies
the pin override rather than a generic .session-item-copy rule.
- Around line 131-134: Ensure the provider panel is present before running the
negative regex check: first capture the match for
/id="panel-config-provider"[\s\S]*?<\/template>/ and assert it's truthy (exists)
so the test fails if the panel is missing, then run assert.doesNotMatch against
the captured match[0] to confirm the moved reset block isn't inside that panel;
reference the regex/id and the test assertion lines around assert.doesNotMatch
in tests/unit/config-tabs-ui.test.mjs.
- Line 343: The current test only checks that a runtime snippet includes "tab
=== 'trash' || tab === 'device'" somewhere, but does not verify
normalizeSettingsTab actually accepts 'device'; update the test to scope the
assertion to normalizeSettingsTab by either (a) importing/locating
normalizeSettingsTab from the bundle and calling it with 'device' and asserting
the result is the expected normalized string, or (b) asserting the source for
the specific function body contains the device branch (e.g., check appScript or
the function string for "function normalizeSettingsTab" and that it includes
"tab === 'device'") — target the symbol normalizeSettingsTab in your change so
the test proves that function, not some other code, handles the 'device' case.
In `@web-ui/partials/index/panel-settings.html`:
- Around line 161-163: The reset button currently calls resetConfig directly,
which risks accidental resets; change the button click handler to call a new
confirmAndResetConfig (leave :disabled using resetConfigLoading || loading ||
!!initError) and implement confirmAndResetConfig in
web-ui/modules/app.methods.providers.mjs to open a confirmation dialog via
this.requestConfirmDialog({ title: '重装配置', message: '该操作会重写本地配置文件。请确认已完成备份。',
confirmText: '继续重装', cancelText: '取消', danger: true }) and only call
this.resetConfig() when the user confirms; keep existing loading/error handling
and return early if not confirmed.
In `@web-ui/styles/sessions-list.css`:
- Around line 317-318: Several CSS rules in sessions-list.css set action button
dimensions to 22px (the width: 22px; height: 22px; declarations) which are below
recommended touch target size; update those declarations to at least 24px (e.g.,
change width and height to 24px) for the selector(s) that define session action
buttons and similarly update the other occurrences noted (the blocks around the
other width/height declarations referenced) so all session action hit targets
are >=24px and visually align with existing padding/margin rules.
🪄 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: d3dbe9b5-942e-42fd-9835-d39443e2870a
📒 Files selected for processing (10)
tests/unit/config-tabs-ui.test.mjstests/unit/web-ui-source-bundle.test.mjsweb-ui/modules/app.methods.session-trash.mjsweb-ui/partials/index/layout-header.htmlweb-ui/partials/index/modal-config-template-agents.htmlweb-ui/partials/index/panel-config-codex.htmlweb-ui/partials/index/panel-settings.htmlweb-ui/styles/modals-core.cssweb-ui/styles/responsive.cssweb-ui/styles/sessions-list.css
💤 Files with no reviewable changes (1)
- web-ui/partials/index/panel-config-codex.html
📜 Review details
🔇 Additional comments (7)
web-ui/partials/index/layout-header.html (1)
246-246: Sessions subtitle copy update is clean and behavior-safe.Line 246 is a text-only adjustment and keeps template logic intact.
web-ui/styles/responsive.css (1)
196-196: Responsive size reductions are consistently applied at the 540px breakpoint.The width/height/icon-size updates are coherent within this media block.
Also applies to: 217-220, 230-231
web-ui/styles/modals-core.css (1)
57-60: Scoped agents modal sizing is well-contained.The new
.agents-modalrules correctly narrow impact to the agents editor flow without affecting other modal editors.Also applies to: 393-397
web-ui/partials/index/modal-config-template-agents.html (1)
28-28:agents-modalclass wiring is correct.This correctly activates the dedicated modal sizing behavior from
web-ui/styles/modals-core.css.web-ui/modules/app.methods.session-trash.mjs (1)
209-212: Settings tab normalization now correctly supportsdevice.This keeps
'device'stable while preserving fallback-to-backupfor unknown values.tests/unit/web-ui-source-bundle.test.mjs (1)
25-25: Good bundled-HTML regression check for the new device panel.The added assertion closes a gap for template inlining coverage.
web-ui/partials/index/panel-settings.html (1)
30-39: New “设备” subtab integration is ARIA-consistent.Tab id,
aria-controls,aria-selected, and active-state wiring are all aligned.
| assert.doesNotMatch( | ||
| html.match(/id="panel-config-provider"[\s\S]*?<\/template>/)?.[0] || '', | ||
| /<span class="selector-title">配置重置<\/span>/ | ||
| ); |
There was a problem hiding this comment.
Assert the provider panel exists before the negative check.
Line 132 falls back to '', so this passes if panel-config-provider disappears or the regex stops before the moved reset block. That weakens the regression this block is trying to add.
🔧 Suggested change
+ const providerPanel = html.match(/id="panel-config-provider"[\s\S]*?<\/template>/);
+ assert(providerPanel, 'provider config panel should exist');
assert.doesNotMatch(
- html.match(/id="panel-config-provider"[\s\S]*?<\/template>/)?.[0] || '',
+ providerPanel[0],
/<span class="selector-title">配置重置<\/span>/
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert.doesNotMatch( | |
| html.match(/id="panel-config-provider"[\s\S]*?<\/template>/)?.[0] || '', | |
| /<span class="selector-title">配置重置<\/span>/ | |
| ); | |
| const providerPanel = html.match(/id="panel-config-provider"[\s\S]*?<\/template>/); | |
| assert(providerPanel, 'provider config panel should exist'); | |
| assert.doesNotMatch( | |
| providerPanel[0], | |
| /<span class="selector-title">配置重置<\/span>/ | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/config-tabs-ui.test.mjs` around lines 131 - 134, Ensure the
provider panel is present before running the negative regex check: first capture
the match for /id="panel-config-provider"[\s\S]*?<\/template>/ and assert it's
truthy (exists) so the test fails if the panel is missing, then run
assert.doesNotMatch against the captured match[0] to confirm the moved reset
block isn't inside that panel; reference the regex/id and the test assertion
lines around assert.doesNotMatch in tests/unit/config-tabs-ui.test.mjs.
| assert.match(appScript, /sessionTrashHasMoreItems\(\)/); | ||
| assert.match(appScript, /sessionTrashHiddenCount\(\)/); | ||
| assert.match(appScript, /normalizeSettingsTab\(tab\)/); | ||
| assert.match(appScript, /tab === 'trash' \|\| tab === 'device'/); |
There was a problem hiding this comment.
Scope the device branch check to normalizeSettingsTab().
Line 343 only proves that this snippet exists somewhere in the bundle. Another method could satisfy it while normalizeSettingsTab() still rejects 'device'.
🔧 Suggested change
- assert.match(appScript, /normalizeSettingsTab\(tab\)/);
- assert.match(appScript, /tab === 'trash' \|\| tab === 'device'/);
+ assert.match(
+ appScript,
+ /normalizeSettingsTab\(tab\)[\s\S]*tab === 'trash' \|\| tab === 'device'/
+ );🤖 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 343, The current test only checks
that a runtime snippet includes "tab === 'trash' || tab === 'device'" somewhere,
but does not verify normalizeSettingsTab actually accepts 'device'; update the
test to scope the assertion to normalizeSettingsTab by either (a)
importing/locating normalizeSettingsTab from the bundle and calling it with
'device' and asserting the result is the expected normalized string, or (b)
asserting the source for the specific function body contains the device branch
(e.g., check appScript or the function string for "function
normalizeSettingsTab" and that it includes "tab === 'device'") — target the
symbol normalizeSettingsTab in your change so the test proves that function, not
some other code, handles the 'device' case.
| /@media \(max-width: 540px\)\s*\{[\s\S]*\.session-item-copy\.session-item-pin svg,\s*[\s\S]*width:\s*16px;/ | ||
| ); | ||
| assert.match(styles, /@media \(max-width: 540px\)\s*\{[\s\S]*\.session-item-copy\s*\{[\s\S]*width:\s*44px;[\s\S]*height:\s*44px;[\s\S]*min-width:\s*44px;[\s\S]*min-height:\s*44px;/); | ||
| assert.match(styles, /@media \(max-width: 540px\)\s*\{[\s\S]*\.session-item-copy\s*\{[\s\S]*width:\s*36px;[\s\S]*height:\s*36px;[\s\S]*min-width:\s*36px;[\s\S]*min-height:\s*36px;/); |
There was a problem hiding this comment.
The mobile size check no longer proves the pin action was shrunk.
Line 431 now matches a generic .session-item-copy rule, but the surrounding assertions are about .session-item-copy.session-item-pin. A more specific pin override can slip through while this still passes.
🤖 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 431, The mobile-size regex is too
broad—it's matching any .session-item-copy rule and can miss a specific pin
override; update the assertion to target the pin selector by changing the
pattern to look for .session-item-copy.session-item-pin (or the exact selector
order used in the stylesheet) within the `@media` (max-width: 540px) block and
assert the width/height/min-width/min-height values for that selector so the
test verifies the pin override rather than a generic .session-item-copy rule.
| <button class="btn-tool" @click="resetConfig" :disabled="resetConfigLoading || loading || !!initError"> | ||
| {{ resetConfigLoading ? '重装中...' : '重装配置' }} | ||
| </button> |
There was a problem hiding this comment.
Add a confirmation step before config reset.
Line 161 triggers resetConfig directly; in web-ui/modules/app.methods.providers.mjs (Line 191 onward), that call immediately invokes api('reset-config'). This creates a high accidental-reset risk from a single misclick.
Suggested guard pattern
-<button class="btn-tool" `@click`="resetConfig" :disabled="resetConfigLoading || loading || !!initError">
+<button class="btn-tool" `@click`="confirmAndResetConfig" :disabled="resetConfigLoading || loading || !!initError">// In web-ui/modules/app.methods.providers.mjs
async confirmAndResetConfig() {
const confirmed = await this.requestConfirmDialog({
title: '重装配置',
message: '该操作会重写本地配置文件。请确认已完成备份。',
confirmText: '继续重装',
cancelText: '取消',
danger: true
});
if (!confirmed) return;
await this.resetConfig();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-ui/partials/index/panel-settings.html` around lines 161 - 163, The reset
button currently calls resetConfig directly, which risks accidental resets;
change the button click handler to call a new confirmAndResetConfig (leave
:disabled using resetConfigLoading || loading || !!initError) and implement
confirmAndResetConfig in web-ui/modules/app.methods.providers.mjs to open a
confirmation dialog via this.requestConfirmDialog({ title: '重装配置', message:
'该操作会重写本地配置文件。请确认已完成备份。', confirmText: '继续重装', cancelText: '取消', danger: true })
and only call this.resetConfig() when the user confirms; keep existing
loading/error handling and return early if not confirmed.
| width: 22px; | ||
| height: 22px; |
There was a problem hiding this comment.
Increase session action hit targets to at least 24px.
Line 317 and Line 318 set the action button to 22px, which is below common accessibility minimum target sizing and makes click/tap precision harder.
Proposed CSS adjustment
.session-item-copy {
- width: 22px;
- height: 22px;
+ width: 24px;
+ height: 24px;
+ min-width: 24px;
+ min-height: 24px;
}
.session-item-copy svg {
- width: 10px;
- height: 10px;
+ width: 12px;
+ height: 12px;
}
.session-item-pin .pin-icon,
.session-item-pin svg {
- width: 9px;
- height: 9px;
+ width: 10px;
+ height: 10px;
}Also applies to: 342-344, 350-354
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-ui/styles/sessions-list.css` around lines 317 - 318, Several CSS rules in
sessions-list.css set action button dimensions to 22px (the width: 22px; height:
22px; declarations) which are below recommended touch target size; update those
declarations to at least 24px (e.g., change width and height to 24px) for the
selector(s) that define session action buttons and similarly update the other
occurrences noted (the blocks around the other width/height declarations
referenced) so all session action hit targets are >=24px and visually align with
existing padding/margin rules.
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)
tests/unit/web-ui-behavior-parity.test.mjs (1)
322-347:⚠️ Potential issue | 🔴 CriticalEmpty data keys allowlist conflicts with still-present health-check data keys, causing pipeline failure.
The pipeline failure indicates the test is failing because
allowedExtraCurrentKeysis now empty (line 322), but the app'sdata()function still declares health-check dialog data keys (perweb-ui/app.js:196-201):
healthCheckDialogLockedProviderhealthCheckDialogSelectedProviderhealthCheckDialogPrompthealthCheckDialogMessageshealthCheckDialogSendinghealthCheckDialogLastResultThese keys will appear as
extraCurrentKeysif they exist in the current bundle but not in the parity baseline, causing the assertion at line 332 to fail.There's also an inconsistency: the method keys allowlist (lines 342-347) still includes health-check methods, but the corresponding data keys allowlist was emptied.
To fix, either:
- Restore the data keys to
allowedExtraCurrentKeysif they are intentionally present in current but not baseline- Remove the data keys from the app if they should no longer exist
Proposed fix (option 1 - restore allowlist)
- const allowedExtraCurrentKeys = []; + const allowedExtraCurrentKeys = [ + 'healthCheckDialogLockedProvider', + 'healthCheckDialogMessages', + 'healthCheckDialogLastResult', + 'healthCheckDialogPrompt', + 'healthCheckDialogSelectedProvider', + 'healthCheckDialogSending' + ];🤖 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 322 - 347, Test fail: allowedExtraCurrentKeys is empty but the app still defines health-check data keys causing unexpected extra keys; update the test to restore those data keys in the allowlist. In the test file, add the health-check data keys (healthCheckDialogLockedProvider, healthCheckDialogSelectedProvider, healthCheckDialogPrompt, healthCheckDialogMessages, healthCheckDialogSending, healthCheckDialogLastResult) to the allowedExtraCurrentKeys array so extraCurrentKeys no longer triggers the assertion; verify consistency with the existing allowedExtraCurrentMethodKeys (buildDefaultHealthCheckPrompt, closeHealthCheckDialog, openHealthCheckDialog, sendHealthCheckDialogMessage) and keep the allowlists aligned with web-ui/app.js's data() and methods.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tests/unit/web-ui-behavior-parity.test.mjs`:
- Around line 322-347: Test fail: allowedExtraCurrentKeys is empty but the app
still defines health-check data keys causing unexpected extra keys; update the
test to restore those data keys in the allowlist. In the test file, add the
health-check data keys (healthCheckDialogLockedProvider,
healthCheckDialogSelectedProvider, healthCheckDialogPrompt,
healthCheckDialogMessages, healthCheckDialogSending,
healthCheckDialogLastResult) to the allowedExtraCurrentKeys array so
extraCurrentKeys no longer triggers the assertion; verify consistency with the
existing allowedExtraCurrentMethodKeys (buildDefaultHealthCheckPrompt,
closeHealthCheckDialog, openHealthCheckDialog, sendHealthCheckDialogMessage) and
keep the allowlists aligned with web-ui/app.js's data() and methods.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 80bd3162-6c14-4396-aed8-6ef688f04161
📒 Files selected for processing (1)
tests/unit/web-ui-behavior-parity.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-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-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.
🪛 GitHub Actions: ci
tests/unit/web-ui-behavior-parity.test.mjs
[error] 354-354: Unit test failed (web UI behavior parity). AssertionError: Expected bundled app skeleton to expose expected data keys drift vs parity baseline. Actual keys array was []. Expected: ['buildDefaultHealthCheckPrompt','closeHealthCheckDialog','openHealthCheckDialog','sendHealthCheckDialogMessage'].
|
@coderabbitai 如果我直接合并会带来回归性异常吗 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me do a quick inspection of the key risk areas before giving you a definitive answer. 🏁 Script executed: Length of output: 7133
✅ 主要风险点：「配置重置」功能迁移这是本次最大的行为变更——将「重装配置」按钮从 核查结果：
结论：功能为干净迁移，未丢失。 ✅
|
| 风险项 | 级别 | 说明 |
|---|---|---|
| 配置重置功能 | 🟢 低 | 完整迁移，链路验证通过 |
| normalizeSettingsTab | 🟢 低 | 增量修复 |
| Parity 测试收紧 | 🟡 中 | 可能导致 CI 失败，建议先跑单测确认 |
| CSS 尺寸调整 | 🟢 低 | 纯视觉变更 |
合并前建议执行一次 node tests/unit/run.mjs 确认所有单测绿灯，通过后即可安全合并，无明显运行时回归风险。
🧠 Learnings used
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-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-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.
2 participants
Summary
Tests
Summary by CodeRabbit
New Features
Style
UI Updates