refactor(web-ui): split session trash methods out of app#66
refactor(web-ui): split session trash methods out of app#66awsl233777 wants to merge 2 commits intomainfrom
Conversation
|
@coderabbitai re-review |
|
Please re-iterate: All suggestions must stay within the existing design. No breaking changes. 🧠 Learnings used✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughThis PR refactors session-trash UI logic from Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.42.0)web-ui/app.jsComment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/session-trash-state.test.mjs`:
- Around line 18-29: The current unit helpers import session-trash.methods.mjs
directly which tests the extracted functions (createSessionTrashMethods,
resolveSessionTrashListLimit, resolveSessionTrashPageSize,
DEFAULT_SESSION_TRASH_LIST_LIMIT, DEFAULT_SESSION_TRASH_PAGE_SIZE) but no longer
verifies that web-ui/app.js actually binds those methods to the UI; add back a
single app-level smoke test that imports the real web-ui/app.js (or requires the
app bootstrap) and asserts that the UI instance exposes the expected
session-trash methods (e.g. createSessionTrashMethods-bound helpers or named
methods) so the extraction wiring is validated end-to-end.
In `@web-ui/modules/session-trash.methods.mjs`:
- Around line 303-309: The normalizeSessionTrashTotalCount helper can return a
floored server value smaller than the currently loaded fallback list; update
normalizeSessionTrashTotalCount(totalCount, fallbackItems) so that after
computing numericTotal and flooring it you return at least fallbackCount (i.e.
Math.max(flooredNumericTotal, fallbackCount)) — keep the existing invalid-number
fallback behavior but ensure the final return value is never less than the
length of fallbackItems to avoid under-reporting sessionTrashCount.
In `@web-ui/modules/skills.methods.mjs`:
- Around line 38-42: The conditional guard around resetSkillsTargetState()
causes state to be retained when reopening the same target; change
openSkillsManager to always call this.resetSkillsTargetState() after setting
this.skillsTargetApp (i.e., remove the targetChanged conditional and keep
assigning this.skillsTargetApp = targetApp then unconditionally invoke
this.resetSkillsTargetState()), referencing the symbols targetApp,
this.skillsTargetApp, targetChanged, and resetSkillsTargetState to locate and
update the code.
🪄 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: 74639b43-b20e-448c-af29-5efda857549e
📒 Files selected for processing (8)
README.en.mdsite/guide/getting-started.mdtests/unit/config-tabs-ui.test.mjstests/unit/session-trash-state.test.mjstests/unit/web-run-host.test.mjsweb-ui/app.jsweb-ui/modules/session-trash.methods.mjsweb-ui/modules/skills.methods.mjs
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-30T04:47:15.339Z
Learning: In PR `#55` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-31T12:37:50.988Z
Learning: In PR `#59` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: SurviveM
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-29T02:42:32.761Z
Learning: The repository maintainer SurviveM (GitHub: SurviveM, repo: SakuraByteCore/codexmate) prefers that CodeRabbit review suggestions do not introduce breaking changes. Reviews should focus on correctness, safety, and quality without recommending refactors or changes that would break existing APIs, interfaces, or behavior.
🔇 Additional comments (7)
tests/unit/web-run-host.test.mjs (2)
112-112: Regex update improves test resilience without changing intent.This match pattern is appropriately tolerant to harmless formatting variance in
cli.jswhile still targetingDEFAULT_WEB_HOSTprecisely.Based on learnings: no breaking changes in suggestions; focus on correctness/safety/quality within existing design.
201-201: Good coverage addition for explicittargetrejection path.This assertion correctly protects the explicit-invalid-target branch and aligns with current
resolveSkillTargetsemantics.README.en.md (2)
127-129: Security messaging and default host wording are clear and accurate.The localhost-default statement plus explicit LAN exposure warning is precise and user-protective.
237-237: Environment variable default now correctly reflects runtime behavior.This table entry is consistent with the current default host value and reduces operator confusion.
site/guide/getting-started.md (1)
30-32: 中文启动说明更新到位，且安全提示完整。默认监听地址与局域网显式开启方式说明一致、清晰，能有效降低误暴露风险。
web-ui/modules/skills.methods.mjs (1)
162-162: Good change: explicit busy-state return value.Returning
falseon the busy early-exit path makes the method contract more consistent and avoids ambiguousundefinedhandling.tests/unit/session-trash-state.test.mjs (1)
1979-1986: Good contract coverage for the new module exports.Checking the exported defaults and resolver helpers directly is a useful guardrail for the extracted module surface.
| const sessionTrashModule = await import(pathToFileURL(path.join(__dirname, '..', '..', 'web-ui', 'modules', 'session-trash.methods.mjs')).href); | ||
| const { | ||
| createSessionTrashMethods, | ||
| resolveSessionTrashListLimit, | ||
| resolveSessionTrashPageSize, | ||
| DEFAULT_SESSION_TRASH_LIST_LIMIT, | ||
| DEFAULT_SESSION_TRASH_PAGE_SIZE | ||
| } = sessionTrashModule; | ||
|
|
||
| function createSessionTrashTestMethods(api, constants = {}) { | ||
| return createSessionTrashMethods({ api, constants }); | ||
| } |
There was a problem hiding this comment.
Keep one app-level smoke test for the extraction wiring.
These helpers switch the session-trash coverage from parsing web-ui/app.js to importing session-trash.methods.mjs directly. That validates the extracted module in isolation, but this file no longer proves that web-ui/app.js actually binds those methods into the UI instance, which is the risky part of this PR. A broken import/binding in web-ui/app.js could now pass here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/session-trash-state.test.mjs` around lines 18 - 29, The current
unit helpers import session-trash.methods.mjs directly which tests the extracted
functions (createSessionTrashMethods, resolveSessionTrashListLimit,
resolveSessionTrashPageSize, DEFAULT_SESSION_TRASH_LIST_LIMIT,
DEFAULT_SESSION_TRASH_PAGE_SIZE) but no longer verifies that web-ui/app.js
actually binds those methods to the UI; add back a single app-level smoke test
that imports the real web-ui/app.js (or requires the app bootstrap) and asserts
that the UI instance exposes the expected session-trash methods (e.g.
createSessionTrashMethods-bound helpers or named methods) so the extraction
wiring is validated end-to-end.
| normalizeSessionTrashTotalCount(totalCount, fallbackItems = this.sessionTrashItems) { | ||
| const fallbackCount = Array.isArray(fallbackItems) ? fallbackItems.length : 0; | ||
| const numericTotal = Number(totalCount); | ||
| if (!Number.isFinite(numericTotal) || numericTotal < 0) { | ||
| return fallbackCount; | ||
| } | ||
| return Math.floor(numericTotal); |
There was a problem hiding this comment.
Keep the normalized trash count at least as large as the loaded fallback list.
This helper now returns the raw floored server value, which means it can shrink below fallbackItems.length. web-ui/app.js Lines 1868-1880 still build the next count from normalizeSessionTrashTotalCount(this.sessionTrashTotalCount, this.sessionTrashItems) + 1, so a stale 0 now becomes 1 even when the UI already has more trash rows loaded. That under-reports sessionTrashCount and can drive the wrong empty/clear-all state.
Minimal fix
normalizeSessionTrashTotalCount(totalCount, fallbackItems = this.sessionTrashItems) {
const fallbackCount = Array.isArray(fallbackItems) ? fallbackItems.length : 0;
const numericTotal = Number(totalCount);
if (!Number.isFinite(numericTotal) || numericTotal < 0) {
return fallbackCount;
}
- return Math.floor(numericTotal);
+ return Math.max(fallbackCount, Math.floor(numericTotal));
},📝 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.
| normalizeSessionTrashTotalCount(totalCount, fallbackItems = this.sessionTrashItems) { | |
| const fallbackCount = Array.isArray(fallbackItems) ? fallbackItems.length : 0; | |
| const numericTotal = Number(totalCount); | |
| if (!Number.isFinite(numericTotal) || numericTotal < 0) { | |
| return fallbackCount; | |
| } | |
| return Math.floor(numericTotal); | |
| normalizeSessionTrashTotalCount(totalCount, fallbackItems = this.sessionTrashItems) { | |
| const fallbackCount = Array.isArray(fallbackItems) ? fallbackItems.length : 0; | |
| const numericTotal = Number(totalCount); | |
| if (!Number.isFinite(numericTotal) || numericTotal < 0) { | |
| return fallbackCount; | |
| } | |
| return Math.max(fallbackCount, Math.floor(numericTotal)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-ui/modules/session-trash.methods.mjs` around lines 303 - 309, The
normalizeSessionTrashTotalCount helper can return a floored server value smaller
than the currently loaded fallback list; update
normalizeSessionTrashTotalCount(totalCount, fallbackItems) so that after
computing numericTotal and flooring it you return at least fallbackCount (i.e.
Math.max(flooredNumericTotal, fallbackCount)) — keep the existing invalid-number
fallback behavior but ensure the final return value is never less than the
length of fallbackItems to avoid under-reporting sessionTrashCount.
| const targetChanged = targetApp !== this.skillsTargetApp; | ||
| if (targetChanged) { | ||
| this.skillsTargetApp = targetApp; | ||
| this.resetSkillsTargetState(); | ||
| } |
There was a problem hiding this comment.
Restore unconditional state reset in openSkillsManager to avoid regression.
Line 39-Line 42 makes resetSkillsTargetState() conditional. With repeated opens on the same target (for example, targetApp: 'codex'), previous state is now retained, which changes user-visible behavior and conflicts with the PR goal to keep behavior unchanged.
Suggested fix (behavior-preserving)
async openSkillsManager(options = {}) {
const targetApp = this.normalizeSkillsTargetApp(options && options.targetApp ? options.targetApp : this.skillsTargetApp);
- const targetChanged = targetApp !== this.skillsTargetApp;
- if (targetChanged) {
+ if (targetApp !== this.skillsTargetApp) {
this.skillsTargetApp = targetApp;
- this.resetSkillsTargetState();
}
+ this.resetSkillsTargetState();
this.showSkillsModal = true;
await this.refreshSkillsList({ silent: false });
},Based on learnings: "no breaking changes in any review suggestion; limit feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const targetChanged = targetApp !== this.skillsTargetApp; | |
| if (targetChanged) { | |
| this.skillsTargetApp = targetApp; | |
| this.resetSkillsTargetState(); | |
| } | |
| async openSkillsManager(options = {}) { | |
| const targetApp = this.normalizeSkillsTargetApp(options && options.targetApp ? options.targetApp : this.skillsTargetApp); | |
| if (targetApp !== this.skillsTargetApp) { | |
| this.skillsTargetApp = targetApp; | |
| } | |
| this.resetSkillsTargetState(); | |
| this.showSkillsModal = true; | |
| await this.refreshSkillsList({ silent: false }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-ui/modules/skills.methods.mjs` around lines 38 - 42, The conditional
guard around resetSkillsTargetState() causes state to be retained when reopening
the same target; change openSkillsManager to always call
this.resetSkillsTargetState() after setting this.skillsTargetApp (i.e., remove
the targetChanged conditional and keep assigning this.skillsTargetApp =
targetApp then unconditionally invoke this.resetSkillsTargetState()),
referencing the symbols targetApp, this.skillsTargetApp, targetChanged, and
resetSkillsTargetState to locate and update the code.
|
Closing this PR for now because the branch has merge conflicts and I’m not continuing this line in this PR. |
1 participant
Summary
web-ui/app.jsintoweb-ui/modules/session-trash.methods.mjsapp.jsand reducing the amount of context future edits need to loadWhy
The goal of this change is not cosmetic cleanup. It is to make future AI-assisted edits cheaper and safer by:
Validation
npm testSummary by CodeRabbit
Documentation
Bug Fixes