refactor(web-ui): extract session trash methods#67
Conversation
|
@coderabbitai re-review |
📝 WalkthroughWalkthroughSession-trash method implementations were moved out of Changes
Sequence Diagram(s)mermaid 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)
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 |
|
✏️ Learnings added
🧠 Learnings used✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unit/config-tabs-ui.test.mjs (1)
145-146: These assertions still verify source text, not the extracted module contract.
readProjectFile()+ regex will still pass ifcreateSessionTrashMethodsstops exporting cleanly, fails to parse, or returns the wrong surface. A small smoke test that imports the factory and exercises one or two methods would give this refactor real regression coverage.Example smoke-test direction
+import { createSessionTrashMethods } from '../../web-ui/modules/session-trash.methods.mjs'; + +test('session trash factory exposes a usable surface', () => { + const methods = createSessionTrashMethods({ + api: async () => ({ items: [], totalCount: 0 }) + }); + + assert.strictEqual(methods.normalizeSettingsTab('trash'), 'trash'); + assert.strictEqual(typeof methods.loadSessionTrash, 'function'); + assert.strictEqual(typeof methods.restoreSessionTrash, 'function'); +});Also applies to: 229-252
🤖 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 145 - 146, The tests currently use readProjectFile() + regex to assert source text, which doesn't verify the actual exported contract; replace those assertions by importing the built module and exercising the factory: dynamically import the module that exports createSessionTrashMethods, call createSessionTrashMethods(...) to get the methods object, assert the presence of expected method names and types, and perform a small smoke call on one method to verify behavior and signature; apply the same change to the other assertions that follow the same pattern (the block that currently mirrors lines 229-252).
🤖 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/session-trash.methods.mjs`:
- Around line 76-81: When sessionTrashLoading is true, instead of returning
immediately after stashing sessionTrashPendingOptions, change the logic in
loadSessionTrash to merge pending options so that "stronger" refresh flags
(e.g., forceRefresh: true) win (use boolean OR for flags rather than last-write
merge), store/return a pending promise (e.g., sessionTrashPendingPromise) that
will resolve only after the queued reload completes, and have the eventual
reload clear the pending promise/options when finished; update callers that
await loadSessionTrash (restore/purge/clear) to await this promise so they
remain busy until the actual refresh runs.
---
Nitpick comments:
In `@tests/unit/config-tabs-ui.test.mjs`:
- Around line 145-146: The tests currently use readProjectFile() + regex to
assert source text, which doesn't verify the actual exported contract; replace
those assertions by importing the built module and exercising the factory:
dynamically import the module that exports createSessionTrashMethods, call
createSessionTrashMethods(...) to get the methods object, assert the presence of
expected method names and types, and perform a small smoke call on one method to
verify behavior and signature; apply the same change to the other assertions
that follow the same pattern (the block that currently mirrors lines 229-252).
🪄 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: 1afe1872-501f-4e68-8f51-d47b6a0febc6
📒 Files selected for processing (4)
tests/unit/config-tabs-ui.test.mjstests/unit/session-trash-state.test.mjsweb-ui/app.jsweb-ui/modules/session-trash.methods.mjs
📜 Review details
🔇 Additional comments (4)
web-ui/app.js (1)
35-35: Nice extraction seam.Spreading the factory back into
methodskeeps the rest ofweb-ui/app.json the samethis.*surface while shrinking the monolithic file.Also applies to: 3204-3210
tests/unit/session-trash-state.test.mjs (3)
4-4: ESM module loading update looks correct.Using
pathToFileURL(...).hreffor dynamic import is a solid cross-platform approach here, and the module destructuring is clean.Also applies to: 17-18
200-202: Good test-factory abstraction.
createSessionTrashTestMethodsis a clean wrapper that reduces repetition and keeps the module boundary explicit in tests.
537-539: Session-trash tests are well migrated to the extracted module surface.These updates consistently exercise
createSessionTrashMethodsvia the helper and keep constants wiring explicit where needed, which aligns well with the behavior-preserving refactor objective.Also applies to: 585-589, 653-654, 719-719, 748-763, 1726-1729, 1892-1898, 1969-1976
| if (this.sessionTrashLoading) { | ||
| this.sessionTrashPendingOptions = { | ||
| ...(this.sessionTrashPendingOptions || {}), | ||
| ...(options || {}) | ||
| }; | ||
| return; |
There was a problem hiding this comment.
Queued reloads shouldn’t resolve before the refresh actually runs.
When sessionTrashLoading is already true, this branch returns immediately after stashing sessionTrashPendingOptions. The callers at Lines 139, 174, and 206 await loadSessionTrash(), so restore/purge/clear can drop their busy state before the queued refresh runs. The last-write-wins merge also lets a later { forceRefresh: false } overwrite an earlier forced refresh, which can re-show stale cached trash state after a mutation.
The queued path should preserve the strongest refresh flags and return a promise that resolves only after the pending reload completes.
🤖 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 76 - 81, When
sessionTrashLoading is true, instead of returning immediately after stashing
sessionTrashPendingOptions, change the logic in loadSessionTrash to merge
pending options so that "stronger" refresh flags (e.g., forceRefresh: true) win
(use boolean OR for flags rather than last-write merge), store/return a pending
promise (e.g., sessionTrashPendingPromise) that will resolve only after the
queued reload completes, and have the eventual reload clear the pending
promise/options when finished; update callers that await loadSessionTrash
(restore/purge/clear) to await this promise so they remain busy until the actual
refresh runs.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
web-ui/modules/session-trash.methods.mjs (1)
75-81:⚠️ Potential issue | 🟠 MajorQueued trash reload returns before the queued refresh actually finishes.
When
loadSessionTrash()is called during an in-flight load, Lines 76-81 only stashsessionTrashPendingOptionsand return. The mutation handlers at Lines 139, 174, and 206 allawaitthis method, so their busy state is cleared before the forced refresh actually runs. The plain spread merge also lets a later{ forceRefresh: false }overwrite an earlier forced refresh, which can leave stale trash data visible after restore/purge/clear. Please preserve the strongest queued flags and return a promise that resolves only after the queued reload kicked off from Lines 117-120 completes.Also applies to: 117-120
🤖 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 75 - 81, The loadSessionTrash function is returning early when a load is in-flight, which only stashes sessionTrashPendingOptions and lets callers proceed before the queued reload finishes; fix by making the queue preserve the strongest flags (e.g., merge forceRefresh with OR so true wins) and return a promise that resolves when the queued reload actually completes. Concretely: in loadSessionTrash, instead of a plain spread merge into sessionTrashPendingOptions, merge options so boolean flags like forceRefresh use logical OR (forceRefresh = (existing.forceRefresh || new.forceRefresh)), store a sessionTrashPendingPromise when enqueuing the pending load, and return that promise; when the in-flight load finishes and the code re-invokes loadSessionTrash with sessionTrashPendingOptions (the existing queued-reload kickoff block), resolve sessionTrashPendingPromise only after that reload completes and clear sessionTrashPendingOptions/sessionTrashPendingPromise. Ensure you reference and update the existing symbols: loadSessionTrash, sessionTrashLoading, sessionTrashPendingOptions and add/use sessionTrashPendingPromise so awaiting callers only proceed after the actual queued reload finishes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@web-ui/modules/session-trash.methods.mjs`:
- Around line 75-81: The loadSessionTrash function is returning early when a
load is in-flight, which only stashes sessionTrashPendingOptions and lets
callers proceed before the queued reload finishes; fix by making the queue
preserve the strongest flags (e.g., merge forceRefresh with OR so true wins) and
return a promise that resolves when the queued reload actually completes.
Concretely: in loadSessionTrash, instead of a plain spread merge into
sessionTrashPendingOptions, merge options so boolean flags like forceRefresh use
logical OR (forceRefresh = (existing.forceRefresh || new.forceRefresh)), store a
sessionTrashPendingPromise when enqueuing the pending load, and return that
promise; when the in-flight load finishes and the code re-invokes
loadSessionTrash with sessionTrashPendingOptions (the existing queued-reload
kickoff block), resolve sessionTrashPendingPromise only after that reload
completes and clear sessionTrashPendingOptions/sessionTrashPendingPromise.
Ensure you reference and update the existing symbols: loadSessionTrash,
sessionTrashLoading, sessionTrashPendingOptions and add/use
sessionTrashPendingPromise so awaiting callers only proceed after the actual
queued reload finishes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3b9a9d8c-7521-46b4-a940-125424365d34
📒 Files selected for processing (4)
tests/unit/config-tabs-ui.test.mjstests/unit/session-trash-state.test.mjsweb-ui/app.jsweb-ui/modules/session-trash.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-04-03T10:42:22.344Z
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:50.988Z
Learning: In PR `#59` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-30T04:47:15.339Z
Learning: In PR `#55` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: SurviveM
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-29T02:42:32.761Z
Learning: The repository maintainer SurviveM (GitHub: SurviveM, repo: SakuraByteCore/codexmate) prefers that CodeRabbit review suggestions do not introduce breaking changes. Reviews should focus on correctness, safety, and quality without recommending refactors or changes that would break existing APIs, interfaces, or behavior.
🔇 Additional comments (4)
tests/unit/config-tabs-ui.test.mjs (1)
146-252: LGTM!The test correctly validates the refactored module structure:
- Confirms
app.jsnow defines and spreadscreateSessionTrashMethods- Verifies the session-trash method signatures exist in the new
session-trash.methods.mjsmoduleThe pattern assertions appropriately track where each piece of functionality now resides.
tests/unit/session-trash-state.test.mjs (3)
4-4: LGTM!The dynamic import pattern correctly uses
pathToFileURLto convert the file path to a URL, which is the proper approach for ESM dynamic imports in Node.js. The top-level await is valid given the.mjsextension.Also applies to: 17-18
200-202: LGTM!Clean helper function that provides a consistent interface for tests to obtain session-trash methods from the new module factory.
537-539: LGTM!The test updates consistently use
createSessionTrashTestMethods()to obtain methods from the new module instead of extracting them fromapp.jssource strings. The test logic and assertions remain unchanged, ensuring behavior-preserving coverage of the refactored code.Also applies to: 584-589, 653-654, 718-719, 746-763, 892-898, 969-976, 1726-1729
2 participants
Summary
web-ui/app.jsintoweb-ui/modules/session-trash.methods.mjsWhy
This change is intentionally aimed at lowering future AI edit cost rather than doing cosmetic cleanup.
It improves that by:
Validation
npm testSummary by CodeRabbit