fix: stabilize web ui startup and reduce usage load#83
Conversation
📝 WalkthroughWalkthroughAdds lightweight session inventory/browse with cached lookups and a bounded tail-based JSONL preview extractor; makes session-list rendering incremental and preview-aware; defers Web UI initial load and probes UI readiness before auto-opening the browser; updates UI assets, templates, styles, and extensive unit/e2e tests. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI process
participant Web as Local Web Server (/api)
participant OS as Platform (exec)
participant FS as Filesystem
CLI->>Web: POST /api { action: "health-check", params: {} }
alt 200 OK
Web-->>CLI: 200 OK
CLI->>OS: exec(open browser URL)
else non-200 / timeout
CLI-->>CLI: schedule retry (setTimeout ms=150)
CLI->>Web: POST /api { action: "health-check" } (retry)
alt eventually 200
Web-->>CLI: 200 OK
CLI->>OS: exec(open browser URL)
else exhausted
CLI-->>OS: skip open
end
end
Note right of CLI: session browse & preview flow
CLI->>FS: listSessionInventoryBySource(browseLightweight)
FS-->>CLI: inventory (cached or scanned)
CLI->>FS: resolveSessionFilePath(sessionId) (lookup-cache preferred)
FS-->>CLI: file path
CLI->>FS: extractSessionDetailPreviewFromFileFast(path, preview:true, budgets)
FS-->>CLI: preview (tail-based, clipped/total info)
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)
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: 3
🧹 Nitpick comments (2)
web-ui/app.js (1)
414-447: Extract the startup defer delay into a named constant.Line 423 and Line 431 both use
120directly. A local constant keeps this easier to tune and aligns the two branches.♻️ Proposed refactor
+ const INITIAL_LOAD_DELAY_MS = 120; const runInitialLoad = () => { const triggerLoad = () => { this._initialLoadTimer = 0; void this.loadAll(); }; if (typeof requestAnimationFrame === 'function') { this._initialLoadRafId = requestAnimationFrame(() => { this._initialLoadRafId = 0; if (typeof setTimeout === 'function') { - this._initialLoadTimer = setTimeout(triggerLoad, 120); + this._initialLoadTimer = setTimeout(triggerLoad, INITIAL_LOAD_DELAY_MS); return; } triggerLoad(); }); return; } if (typeof setTimeout === 'function') { - this._initialLoadTimer = setTimeout(triggerLoad, 120); + this._initialLoadTimer = setTimeout(triggerLoad, INITIAL_LOAD_DELAY_MS); return; } triggerLoad(); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/app.js` around lines 414 - 447, Extract the hard-coded delay used for initial startup deferral into a named constant (e.g., const INITIAL_LOAD_DELAY = 120) and use that constant instead of the literal 120 in both places inside runInitialLoad where setTimeout is called (referencing the triggerLoad invocation and this._initialLoadTimer assignment within runInitialLoad). Ensure the constant is declared in the same scope as runInitialLoad so both branches (requestAnimationFrame and fallback setTimeout) reference the same value and update any comments if present.tests/unit/session-usage-backend.test.mjs (1)
82-115: Consider adding one edge-case test for non-array upstream responses.You’re already validating normalization well; adding this case will lock in the
[]fallback behavior and prevent regressions there.Suggested test addition
+test('listSessionUsage returns [] when listAllSessions returns non-array data', async () => { + const listSessionUsage = instantiateListSessionUsage({ + MAX_SESSION_LIST_SIZE: 300, + listAllSessions: async () => ({ unexpected: true }), + listAllSessionsData: async () => { + throw new Error('should not call listAllSessionsData'); + } + }); + + const result = await listSessionUsage({ source: 'all', limit: 10, forceRefresh: false }); + assert.deepStrictEqual(result, []); +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/session-usage-backend.test.mjs` around lines 82 - 115, Add an edge-case unit test to verify that listSessionUsage (created via instantiateListSessionUsage) handles non-array upstream responses by falling back to an empty array; specifically, add a test that stubs listAllSessions or listAllSessionsData to return a non-array value (e.g., null or an object) and assert that the function still calls listAllSessions with normalized params and does not throw, preserving the existing [] fallback behavior in listSessionUsage/listAllSessionsData handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli.js`:
- Around line 10779-10809: The readiness probe in the client sends a POST to
/api with action:'status' and causes the server-side status handler (which calls
consumeInitNotice()) to clear initNotice before the UI reads it; to fix, modify
the probe or server action handling so the probe is side-effect free: either
change the probe payload/action or add a branch in the /api action switch (the
server-side handler that inspects action === 'status' and calls
consumeInitNotice()) to return status without calling consumeInitNotice() for
requests coming from the readiness probe (identify probe via a distinct action
value or an extra header/query flag), and update the probe code
(payload/requestOptions in the client: the JSON payload variable and
http.request usage like req.end(payload, 'utf-8')) to use that distinct,
non-destructive action or header so the frontend’s first real status fetch still
receives the initNotice.
- Around line 10824-10836: The current code builds a shell string using the
user-controlled url and calls exec(command...), which allows shell injection;
replace this with child_process.spawn to pass the URL as an argument (shell:
false) instead of interpolating it. Specifically, stop constructing the string
in the platform branches and instead spawn the platform-specific binary with the
URL as an argument (e.g. on darwin spawn('open', [url]), on Linux
spawn('xdg-open', [url]), and on Windows spawn('cmd', ['/c', 'start', '', url])
or equivalent), handle the spawned process' error/exit events in place of the
exec callback, and ensure you import/require spawn from 'child_process'; update
the code that currently references platform, command, and exec(command, (error)
=> { ... }) accordingly.
In `@web-ui/index.html`:
- Line 9: The script tag in web-ui/index.html currently references the prod
runtime "/res/vue.global.prod.js" which conflicts with the E2E test expectations
in tests/e2e/test-web-ui-assets.js that assert the compiler-enabled
"/res/vue.global.js"; change the <script src="..."> back to "/res/vue.global.js"
to restore the compiler-enabled runtime (or, alternatively, update the
assertions in tests/e2e/test-web-ui-assets.js to accept the prod variant),
ensuring consistency between web-ui/index.html's script src and the test
expectations.
---
Nitpick comments:
In `@tests/unit/session-usage-backend.test.mjs`:
- Around line 82-115: Add an edge-case unit test to verify that listSessionUsage
(created via instantiateListSessionUsage) handles non-array upstream responses
by falling back to an empty array; specifically, add a test that stubs
listAllSessions or listAllSessionsData to return a non-array value (e.g., null
or an object) and assert that the function still calls listAllSessions with
normalized params and does not throw, preserving the existing [] fallback
behavior in listSessionUsage/listAllSessionsData handling.
In `@web-ui/app.js`:
- Around line 414-447: Extract the hard-coded delay used for initial startup
deferral into a named constant (e.g., const INITIAL_LOAD_DELAY = 120) and use
that constant instead of the literal 120 in both places inside runInitialLoad
where setTimeout is called (referencing the triggerLoad invocation and
this._initialLoadTimer assignment within runInitialLoad). Ensure the constant is
declared in the same scope as runInitialLoad so both branches
(requestAnimationFrame and fallback setTimeout) reference the same value and
update any comments if present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9ae24d97-f7aa-4f65-90d7-6a4849864476
📒 Files selected for processing (17)
cli.jstests/unit/compact-layout-ui.test.mjstests/unit/config-tabs-ui.test.mjstests/unit/run.mjstests/unit/session-usage-backend.test.mjstests/unit/web-run-host.test.mjstests/unit/web-ui-source-bundle.test.mjstests/unit/web-ui-startup-init.test.mjsweb-ui/app.jsweb-ui/index.htmlweb-ui/partials/index/layout-header.htmlweb-ui/partials/index/modal-skills.htmlweb-ui/partials/index/panel-docs.htmlweb-ui/partials/index/panel-market.htmlweb-ui/partials/index/panel-usage.htmlweb-ui/styles/navigation-panels.cssweb-ui/styles/sessions-preview.css
📜 Review details
🧰 Additional context used
🪛 GitHub Actions: ci
web-ui/index.html
[error] 1-1: E2E failed: /web-ui/index.html should use the compiler-enabled vue runtime
🔇 Additional comments (16)
web-ui/partials/index/panel-docs.html (1)
11-11: Copy simplification here is clean and consistent with the docs panel flow.No functional regressions in these template edits; the updated wording remains clear and concise.
Also applies to: 80-80, 110-110, 122-122, 124-125
web-ui/partials/index/panel-usage.html (1)
19-19: Loading-state update looks good.Line 19 keeps the same rendering condition while improving wording; scope is minimal and safe.
web-ui/partials/index/layout-header.html (1)
236-236: Subtitle copy refresh is clear and consistent.Line 236 preserves existing tab-based logic and only tightens user-facing text.
web-ui/partials/index/panel-market.html (1)
11-11: Market panel copy changes are cohesive and non-breaking.These edits stay strictly presentational and keep behavior unchanged.
Also applies to: 77-77, 84-84, 102-102, 109-109, 127-127, 133-133, 137-137, 141-141, 149-149, 156-156, 162-162, 167-169
web-ui/partials/index/modal-skills.html (1)
6-6: Skills modal wording updates look good.Copy is shorter while keeping intent intact; no functional impact detected.
Also applies to: 65-65, 125-125
cli.js (1)
5778-5792: Good cache hygiene on the lightweight usage path.Cloning each session before deleting
__messageCountExactavoids mutating the cached objects returned bylistAllSessions().web-ui/app.js (1)
454-465: Unmount cleanup for deferred startup work is correctly handled.The listener/timer/RAF teardown is complete and prevents stale callbacks after unmount.
tests/unit/web-ui-startup-init.test.mjs (1)
7-93: Good regression coverage for startup deferral sequencing.This test cleanly verifies the
load→ RAF → timeout chain and ensures cleanup behavior is exercised.tests/unit/web-run-host.test.mjs (1)
804-1035: Readiness gating tests are strong and targeted.The added cases validate both “don’t auto-open before probe success” and “retry after probe failure” behavior with clear command/timer assertions.
tests/unit/web-ui-source-bundle.test.mjs (1)
27-28: Bundle assertion update is correct.Verifying inclusion of
/res/vue.global.prod.jsand exclusion of/res/vue.global.jsis the right regression guard.tests/unit/run.mjs (1)
34-34: Good test-runner integration.Including the new backend usage test in the central runner closes a common coverage gap.
tests/unit/config-tabs-ui.test.mjs (1)
31-32: These template assertions are aligned with the UI changes.The production Vue script checks plus explicit loading/error/empty usage-state checks are valuable regressions.
Also applies to: 73-75
web-ui/styles/sessions-preview.css (1)
12-12: Spacing adjustments are coherent.The reduced timeline width/offset and matching scroll padding update are consistent with the tighter preview layout goal.
Also applies to: 120-123, 174-175
web-ui/styles/navigation-panels.css (1)
338-339: Layout constraint update looks good.Using
width: min(100%, 1480px)with explicit.mode-content { width: 100%; }gives a clearer, predictable container sizing model.Also applies to: 343-343
tests/unit/compact-layout-ui.test.mjs (1)
58-64: Updated layout assertions look correct and aligned with the CSS changes.These checks appropriately lock in the new desktop width behavior and session preview/timeline spacing updates.
tests/unit/session-usage-backend.test.mjs (1)
31-80: Good regression coverage for lightweight usage retrieval path.This test clearly verifies the expected call path and payload normalization.
|
@coderabbitai If the current suggestion is not adopted, will it lead to disruptive changes? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 43366
🔴 Already causing a CI failure — action needed
The GitHub Actions CI run already reports:
The E2E test in
|
|
@coderabbitai please confirm whether there are any remaining issues on this PR. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 9323
The two remaining items from the earlier review are non-blocking:
The PR is in a clean state and ready for merge. 🎉 🧠 Learnings used |
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 `@cli.js`:
- Around line 5554-5567: listSessionInventoryBySource currently returns cached
session objects by reference which allows later mutation (e.g.,
applySessionQueryFilter adding match) to leak into other responses; fix this by
returning deep clones of cached data and storing clones in the cache so callers
always get independent objects: when calling getSessionInventoryCache(cacheKey,
forceRefresh) return a deep-cloned copy of the sessions, and when storing with
setSessionInventoryCache(cacheKey, normalizedSource, sessions) store a deep
clone of the sessions array/object rather than the original reference (use a
safe deep-clone util or JSON round-trip if structures are plain).
- Around line 4743-4800: The fast preview must only be accepted when we know
it's complete/valid: modify extractSessionDetailPreviewFromFileFast so it never
returns latest unless latest.totalMessages is non-null (i.e., the tail parser
confirmed total count). Concretely, in the while-loop when you would
early-return after reaching safeMessageLimit, only return if
latest.totalMessages !== null; otherwise continue scanning/fallthrough. After
the loop (before the final return latest), if latest.totalMessages === null
return null so callers fall back to the full reader; keep setting latest.clipped
based on position>0 as already done.
In `@web-ui/session-helpers.mjs`:
- Around line 14-19: normalizeSessionLoadOptions currently defaults
includeActiveDetail to false causing loadSessions() and loadSessions({
forceRefresh: true }) to drop activeSessionMessages; update
normalizeSessionLoadOptions (function name: normalizeSessionLoadOptions) so
includeActiveDetail defaults to true unless callers explicitly pass
includeActiveDetail: false (i.e., treat missing/undefined as true, while keeping
forceRefresh behavior unchanged), ensuring existing callers like loadSessions()
and the refresh action preserve detail hydration.
🪄 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: 81d24ef0-0c35-4bea-9630-5d03991b31fd
📒 Files selected for processing (17)
cli.jstests/e2e/test-mcp.jstests/e2e/test-sessions.jstests/unit/config-tabs-ui.test.mjstests/unit/session-browser-timeline-regression.test.mjstests/unit/session-tab-switch-performance.test.mjstests/unit/session-usage-backend.test.mjstests/unit/web-ui-behavior-parity.test.mjstests/unit/web-ui-logic.test.mjstests/unit/web-ui-runtime-navigation-regression.test.mjstests/unit/web-ui-startup-init.test.mjsweb-ui/app.jsweb-ui/modules/app.computed.session.mjsweb-ui/modules/app.methods.navigation.mjsweb-ui/modules/app.methods.session-browser.mjsweb-ui/partials/index/panel-sessions.htmlweb-ui/session-helpers.mjs
✅ Files skipped from review due to trivial changes (2)
- tests/unit/web-ui-logic.test.mjs
- tests/unit/session-browser-timeline-regression.test.mjs
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/unit/config-tabs-ui.test.mjs
- tests/unit/web-ui-startup-init.test.mjs
- tests/unit/session-usage-backend.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-03-30T04:47:15.352Z
Learning: In PR `#55` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-04-04T01:36:26.964Z
Learning: In PR `#68` for SakuraByteCore/codexmate, github-actions[bot] again flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint (reinforced across PRs `#55`, `#59`, `#67`, `#68`) is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-31T12:37:51.023Z
Learning: In PR `#59` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-04-03T10:42:22.368Z
Learning: In PR `#67` for SakuraByteCore/codexmate, github-actions[bot] flagged that CodeRabbit was again introducing breaking changes. The maintainer's absolute constraint (reinforced across PRs `#55`, `#59`, `#67`) is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: SurviveM
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-29T02:42:32.785Z
Learning: The repository maintainer SurviveM (GitHub: SurviveM, repo: SakuraByteCore/codexmate) prefers that CodeRabbit review suggestions do not introduce breaking changes. Reviews should focus on correctness, safety, and quality without recommending refactors or changes that would break existing APIs, interfaces, or behavior.
🪛 Biome (2.4.10)
web-ui/session-helpers.mjs
[error] 210-210: Unsafe usage of 'return'.
(lint/correctness/noUnsafeFinally)
🔇 Additional comments (8)
tests/e2e/test-mcp.js (2)
212-213: Good assertion update for long-session preview counts.This keeps the test resilient to non-exact preview totals while still enforcing type/sanity guarantees (
finiteand>= 0).
227-228: Resource-path assertion change is consistent and correct.The same numeric/non-negative contract here keeps MCP resource validation aligned with the updated preview semantics.
web-ui/modules/app.computed.session.mjs (2)
46-70:visibleSessionsListlogic is robust and well-bounded.The render gate, count sanitization, and active-session inclusion behavior are implemented cleanly and defensively.
81-84: Configurable initial preview batch fallback looks solid.Using
sessionPreviewInitialBatchSizewith integer/minimum guards preserves safe behavior and improves control over first render volume.web-ui/modules/app.methods.navigation.mjs (4)
399-507: LGTM! Well-structured viewport-based rendering implementation.The incremental session list rendering logic is well-designed:
- Proper idle scheduling with cancellation support
- Defensive guards for tab/render state before operations
ensureActivelogic correctly ensures the active session is always visible- Recursive scheduling in
scheduleSessionListViewportFillis properly guarded by state checksThe magic numbers (120, 160) were already noted in PR discussion as deferrable maintainability items.
543-565: LGTM! Proper cleanup integration.The suspend and teardown methods correctly clean up the new viewport rendering state:
- Cancels scheduled viewport fills to prevent work on inactive tabs
- Clears the session list DOM reference to avoid stale refs
- Uses
resetSessionListRender()for consistent state reset
572-584: LGTM! Correct initialization sequence.The preparation flow follows the right order: reset → enable → prime, ensuring a clean state before starting the viewport rendering pipeline.
597-610: LGTM! Good efficiency improvement.Gating
scheduleSessionTimelineSync()onsessionTimelineEnabledprevents unnecessary work when the timeline is disabled. The frame-deferred execution with fallback is a sensible approach for smoother rendering.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cli.js (1)
4790-4793:⚠️ Potential issue | 🟠 MajorFall back when the fast scan never finds a complete tail message.
If the 1 MB budget is exhausted before any complete JSONL message can be reconstructed, this still returns an empty preview object.
readSessionDetail()treats that as success, so large tail records can render as a blank conversation instead of using the existing streaming fallback.Minimal fix
- if (position > 0) { + if (latest.messages.length === 0) { + return null; + } + if (position > 0) { latest.clipped = true; } return latest;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 4790 - 4793, The preview builder currently returns an empty latest object when the fast tail scan exhausts the 1MB budget without reconstructing any complete JSONL message; change the return semantics so that when no complete messages were found (e.g., latest has no entries/content) after the fast scan budget is exhausted, return a failure sentinel (null/undefined) instead of an empty object so readSessionDetail() can trigger the streaming fallback; locate the logic that sets latest/clipped (references: latest, latest.clipped, position) and add a check for “no reconstructed messages && budget exhausted” to return null/undefined rather than latest.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli.js`:
- Around line 4783-4786: The tail parser can already set latest.clipped=true
inside extractSessionDetailPreviewFromTailText when it stopped due to
messageLimit, but the caller overwrites it with latest.clipped = position > 0;
change that assignment to preserve any prior truncation by using latest.clipped
= latest.clipped || (position > 0) (referencing latest.clipped, position,
safeMessageLimit, extractSessionDetailPreviewFromTailText and messageLimit) so
once clipped is true it remains true.
---
Duplicate comments:
In `@cli.js`:
- Around line 4790-4793: The preview builder currently returns an empty latest
object when the fast tail scan exhausts the 1MB budget without reconstructing
any complete JSONL message; change the return semantics so that when no complete
messages were found (e.g., latest has no entries/content) after the fast scan
budget is exhausted, return a failure sentinel (null/undefined) instead of an
empty object so readSessionDetail() can trigger the streaming fallback; locate
the logic that sets latest/clipped (references: latest, latest.clipped,
position) and add a check for “no reconstructed messages && budget exhausted” to
return null/undefined rather than latest.
🪄 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: 666e85e4-fb34-4436-bd90-b6823fa7ff0f
📒 Files selected for processing (4)
cli.jstests/unit/session-tab-switch-performance.test.mjstests/unit/session-usage-backend.test.mjsweb-ui/session-helpers.mjs
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/unit/session-usage-backend.test.mjs
- tests/unit/session-tab-switch-performance.test.mjs
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-04-04T01:36:26.964Z
Learning: In PR `#68` for SakuraByteCore/codexmate, github-actions[bot] again flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint (reinforced across PRs `#55`, `#59`, `#67`, `#68`) is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-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.
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: SurviveM
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-29T02:42:32.785Z
Learning: The repository maintainer SurviveM (GitHub: SurviveM, repo: SakuraByteCore/codexmate) prefers that CodeRabbit review suggestions do not introduce breaking changes. Reviews should focus on correctness, safety, and quality without recommending refactors or changes that would break existing APIs, interfaces, or behavior.
🪛 Biome (2.4.10)
web-ui/session-helpers.mjs
[error] 260-260: Unsafe usage of 'return'.
(lint/correctness/noUnsafeFinally)
🔇 Additional comments (4)
cli.js (4)
11184-11196: Prefer argument-based process launch overexec()here.The browser-open path still interpolates
urlinto a shell command string. Sinceurlis derived from--host/CODEXMATE_HOST, that keeps the same shell-injection surface the earlier review already called out.This read-only check should show the user-controlled host flowing into the
exec(command, ...)path:#!/bin/bash rg -n -C3 'function resolveWebHost|CODEXMATE_HOST|openBrowserAfterReady|exec\(command|formatHostForUrl' cli.js
5500-5576: Good: inventory cache entries are now isolated per request.Cloning on both cache write and cache read avoids leaking
matchmetadata across responses while keeping the existing API shape intact.
6097-6137: Good trade-off on the hot path.These responses still keep
messageCountintact while dropping only__messageCountExact, so the usage UI can continue aggregating and sorting without forcing exact hydration on every request.
11138-11213: Good fix: the readiness probe is now side-effect free.Using
health-checkhere avoids consuminginitNoticebefore the frontend’s first real status poll.Also applies to: 11261-11263
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cli.js (1)
4743-4793:⚠️ Potential issue | 🟠 MajorFast preview still returns unverified results as if they were authoritative.
Line 4783 can still return a preview with
totalMessages === null, and Line 4785 still overwrites an already-trueclippedflag withposition > 0. That means some large-session previews can disagree with the full reader while being reported as complete enough to use.🩹 Minimal correctness fix
latest = extractSessionDetailPreviewFromTailText(combined.toString('utf-8'), source, messageLimit); if (latest.messages.length >= safeMessageLimit) { - latest.clipped = position > 0; - return latest; + latest.clipped = latest.clipped || position > 0; + if (latest.totalMessages !== null) { + return latest; + } } } - if (position > 0) { - latest.clipped = true; - } - return latest; + if (latest.totalMessages === null) { + return null; + } + latest.clipped = latest.clipped || position > 0; + return latest;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 4743 - 4793, In extractSessionDetailPreviewFromFileFast: ensure the preview isn't returned as authoritative when totalMessages is unknown and don't overwrite a true clipped flag; call extractSessionDetailPreviewFromTailText with safeMessageLimit (not messageLimit), change the early-return condition to require latest.totalMessages !== null && latest.messages.length >= safeMessageLimit, and set clipped using logical-or (latest.clipped = latest.clipped || position > 0) both before the early return and at the end so an existing true clipped isn't reset.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli.js`:
- Around line 6122-6136: The usage endpoint (listSessionUsage) is still calling
the heavy listAllSessions path; replace that call with the new lightweight
browse implementation you added (the lightweight counterpart to
listAllSessions—e.g., browseAllSessions or listAllSessionsLight) so the endpoint
uses the cheap browse path while preserving the same params (source, limit,
forceRefresh) and the normalization/map logic that removes __messageCountExact;
keep the Array.isArray check and fallback to [] so response shape is unchanged.
---
Duplicate comments:
In `@cli.js`:
- Around line 4743-4793: In extractSessionDetailPreviewFromFileFast: ensure the
preview isn't returned as authoritative when totalMessages is unknown and don't
overwrite a true clipped flag; call extractSessionDetailPreviewFromTailText with
safeMessageLimit (not messageLimit), change the early-return condition to
require latest.totalMessages !== null && latest.messages.length >=
safeMessageLimit, and set clipped using logical-or (latest.clipped =
latest.clipped || position > 0) both before the early return and at the end so
an existing true clipped isn't reset.
🪄 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: 6bf0459d-bf83-4d23-8ca8-bd0ffd0bad1e
📒 Files selected for processing (2)
cli.jstests/e2e/test-sessions.js
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/test-sessions.js
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-04-04T01:36:26.964Z
Learning: In PR `#68` for SakuraByteCore/codexmate, github-actions[bot] again flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint (reinforced across PRs `#55`, `#59`, `#67`, `#68`) is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-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.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web-ui/session-helpers.mjs`:
- Around line 257-261: The finally block currently reads and clears
this.__sessionPendingLoadOptions and then returns loadSessions(...), which can
swallow exceptions thrown in the try; instead, remove the recursive return from
the finally block: capture pendingOptions from this.__sessionPendingLoadOptions
and clear it inside finally (preserve that behavior), but perform the
conditional call to loadSessions(api, pendingOptions) after the
try/catch/finally has completed so any exception is propagated (or rethrown)
first; update references in this function to __sessionPendingLoadOptions,
pendingOptions and the loadSessions call accordingly.
🪄 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: c0264a31-a1ba-430e-97b5-09e71750f1b8
📒 Files selected for processing (4)
tests/e2e/run.jstests/e2e/test-web-ui-session-browser.jstests/unit/session-tab-switch-performance.test.mjsweb-ui/session-helpers.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/session-tab-switch-performance.test.mjs
📜 Review details
🧰 Additional context used
🪛 Biome (2.4.10)
web-ui/session-helpers.mjs
[error] 260-260: Unsafe usage of 'return'.
(lint/correctness/noUnsafeFinally)
🔇 Additional comments (11)
web-ui/session-helpers.mjs (5)
14-46: LGTM - Context-aware option normalization addresses prior concerns.The combination of
hasIncludeActiveDetailtracking andshouldIncludeActiveSessionDetail(lines 41-46) properly handles the default case: when callers don't explicitly passincludeActiveDetail, the function checksvm.mainTab === 'sessions' || !!vm.sessionStandaloneto determine the appropriate behavior. This preserves detail hydration for existing callers on the sessions tab.
52-55: LGTM - Hydration ticket prevents stale task execution.The incrementing ticket pattern properly guards against race conditions where multiple
scheduleSessionDetailHydrationcalls could queue competing tasks.
289-297: LGTM - Preview parameter correctly scoped to session tab context.The
previewflag appropriately reflects when the user is viewing sessions in the tab (not standalone), enabling the backend to return optimized responses without full message counting overhead.
368-382: LGTM - Timeline sync scheduling has proper staleness guards.The
syncTaskcapturescurrentSessionand verifies both tab state and session identity before invokingscheduleSessionTimelineSync(). Deferring viascheduleAfterFramewhen available avoids unnecessary synchronous work.
459-461: LGTM - Limit clamping condition correctly tightened.Only capping
nextLimittototalKnownwhentotalKnown > totalis logically correct; it avoids artificially constraining the fetch when the known count equals or trails the loaded count.tests/e2e/run.js (1)
27-27: LGTM - Test module integration follows existing patterns.The new session browser E2E test is correctly required and invoked in sequence after
testWebUiAssets.Also applies to: 136-136
tests/e2e/test-web-ui-session-browser.js (5)
1-18: LGTM - Lazy-loaded harness with proper caching.The singleton pattern for
bundledWebUiHarnessPromiseavoids redundant parsing of the bundled web UI script across test runs.
59-82: LGTM - Mock fetch correctly bridges UI requests to real API.Routing through
ctx.apienables testing the actual backend behavior rather than stubbing responses, which provides more meaningful coverage.
99-148: LGTM - Lightweight VM harness correctly mirrors Vue app structure.The stub appropriately copies
data(), bindsmethods, and wirescomputedas getters. Scheduler stubs enable controlled execution without browser APIs.
231-250: LGTM - Assertions thoroughly validate incremental loading behavior.The test verifies that the session browser:
- Stays within the isolated dataset
- Respects initial batch sizes for both list and detail
- Grows detail hydration incrementally without pulling entire large sessions
- Maintains the clipped flag appropriately
252-268: LGTM - Huge-line session regression test validates recovery path.This validates the real UI→API flow for sessions with extremely large messages, ensuring:
- Selection completes without hanging
- Messages are recovered without duplication
- Clipped state is correctly set to false when all messages fit
- Visible count matches actual message count
There was a problem hiding this comment.
♻️ Duplicate comments (3)
cli.js (3)
11184-11196:⚠️ Potential issue | 🟠 MajorAvoid
exec()with an interpolated URL.Line 11194 still shells out with
url, which is built from--host/CODEXMATE_HOST. A crafted host containing quotes or shell metacharacters can escape the command string; pass the URL as an argument viaspawn(..., { shell: false })instead.Suggested fix
- const platform = process.platform; - let command; - if (platform === 'win32') { - command = `start \"\" \"${url}\"`; - } else if (platform === 'darwin') { - command = `open \"${url}\"`; - } else { - command = `xdg-open \"${url}\"`; - } - - exec(command, (error) => { - if (error) console.warn('无法自动打开浏览器，请手动访问:', url); - }); + const commandSpec = process.platform === 'win32' + ? { command: 'cmd', args: ['/c', 'start', '', url] } + : (process.platform === 'darwin' + ? { command: 'open', args: [url] } + : { command: 'xdg-open', args: [url] }); + + const child = spawn(commandSpec.command, commandSpec.args, { + stdio: 'ignore', + detached: true, + windowsHide: true, + shell: false + }); + child.on('error', () => { + console.warn('无法自动打开浏览器，请手动访问:', url); + }); + child.unref();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 11184 - 11196, The current code builds a shell interpolated command using platform/command and calls exec(command, ...) with the interpolated url, which allows shell injection via a crafted CODEXMATE_HOST; change this to use child_process.spawn (or execFile) and pass the URL as a separate argument (do not interpolate into a shell string) and set shell: false so the URL cannot be interpreted by a shell—update the logic around platform, command, and the exec call to call spawn/execFile with args (e.g., ['open', url] on darwin, ['xdg-open', url] on linux, or use 'cmd' with ['/c','start','',url'] on win32) and handle the callback/exit to log the same warning on error.
4743-4793:⚠️ Potential issue | 🟠 MajorNormalize the full-scan fast preview before returning.
When Line 4793 returns
latestafterpositionreaches0, the result still comes straight fromextractSessionDetailPreviewFromTailText(), which never strips leading bootstrap/system messages. That makespreview=truedisagree with the normal reader for large sessions that happen to have fewer thanmessageLimitconversation messages.Suggested fix
- if (position > 0) { - latest.clipped = latest.clipped || position > 0; - } - return latest; + if (position > 0) { + latest.clipped = true; + return latest; + } + latest.messages = removeLeadingSystemMessage(latest.messages); + latest.totalMessages = latest.messages.length; + latest.clipped = false; + return latest;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 4743 - 4793, The fast full-scan return path in extractSessionDetailPreviewFromFileFast currently returns the raw output from extractSessionDetailPreviewFromTailText, which still contains leading bootstrap/system messages; after the loop when position reaches 0, post-process latest to normalize it the same way the normal reader does by removing leading bootstrap/system messages and trimming to safeMessageLimit (i.e., call or implement the same normalization/stripping routine used by the normal reader), ensure latest.messages is recalculated to respect safeMessageLimit and set latest.clipped appropriately (latest.clipped = latest.clipped || position > 0), so preview=true results match the normal reader for large sessions with fewer than messageLimit conversation messages.
6114-6136:⚠️ Potential issue | 🟠 MajorUse the lightweight browse path for usage.
Line 6122 still calls
listAllSessions(), so the usage endpoint keeps the heavier default scan budgets instead of the new browse budgets. On larger session stores, that largely cancels the PR's load-reduction win.Suggested fix
- const sessions = await listAllSessions({ + const sessions = await listSessionBrowse({ source, limit, forceRefresh: !!params.forceRefresh }); - return Array.isArray(sessions) - ? sessions.map((item) => { - if (!item || typeof item !== 'object' || Array.isArray(item)) { - return item; - } - const normalized = { ...item }; - delete normalized.__messageCountExact; - return normalized; - }) - : []; + return Array.isArray(sessions) ? sessions : [];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 6114 - 6136, The usage listing still calls listAllSessions() with default (heavy) scan budgets; update listSessionUsage to invoke the lightweight/browse path by passing the browse flag or using the browse-specific helper—e.g., call listAllSessions({ source, limit, forceRefresh: !!params.forceRefresh, browse: true }) or replace with the equivalent listAllSessionsBrowse(...) helper so the endpoint uses the new lightweight/browse budgets (refer to listSessionUsage and listAllSessions in the diff).
🧹 Nitpick comments (1)
tests/unit/session-detail-preview-fast.test.mjs (1)
15-35: Extract these utility functions to a dedicated test helper module.This test currently parses raw
cli.jssource code using regex to extract four internal functions (applySessionDetailRecordMetadata,extractSessionDetailPreviewFromTailText,extractSessionDetailPreviewFromFileFast,extractMessageFromRecord). The regex pattern only matches traditionalfunctiondeclarations—any refactor to arrow functions, class methods, or even syntax reorganization will break this test despite no behavioral change.These functions are not exported from
cli.js(which is a CLI entry point). Move them to a dedicated test utilities module (e.g.,tests/unit/session-detail-preview-helpers.js) and import directly instead of parsing source. This decouples the test from syntax specifics and makes the actual behavior testable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/session-detail-preview-fast.test.mjs` around lines 15 - 35, The test brittlely extracts internal functions via regex; instead move the four functions applySessionDetailRecordMetadata, extractSessionDetailPreviewFromTailText, extractSessionDetailPreviewFromFileFast, and extractMessageFromRecord into a new test helper module (e.g., tests/unit/session-detail-preview-helpers.js), export them, update this test to import those functions directly, remove the extractFunction/regex logic and the dynamic Function(...) wrapper, and adapt instantiateExtractSessionDetailPreviewFromFileFast to return the imported extractSessionDetailPreviewFromFileFast (wiring any required test bindings via parameters or simple wrapper functions) so the test no longer depends on cli.js source shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cli.js`:
- Around line 11184-11196: The current code builds a shell interpolated command
using platform/command and calls exec(command, ...) with the interpolated url,
which allows shell injection via a crafted CODEXMATE_HOST; change this to use
child_process.spawn (or execFile) and pass the URL as a separate argument (do
not interpolate into a shell string) and set shell: false so the URL cannot be
interpreted by a shell—update the logic around platform, command, and the exec
call to call spawn/execFile with args (e.g., ['open', url] on darwin,
['xdg-open', url] on linux, or use 'cmd' with ['/c','start','',url'] on win32)
and handle the callback/exit to log the same warning on error.
- Around line 4743-4793: The fast full-scan return path in
extractSessionDetailPreviewFromFileFast currently returns the raw output from
extractSessionDetailPreviewFromTailText, which still contains leading
bootstrap/system messages; after the loop when position reaches 0, post-process
latest to normalize it the same way the normal reader does by removing leading
bootstrap/system messages and trimming to safeMessageLimit (i.e., call or
implement the same normalization/stripping routine used by the normal reader),
ensure latest.messages is recalculated to respect safeMessageLimit and set
latest.clipped appropriately (latest.clipped = latest.clipped || position > 0),
so preview=true results match the normal reader for large sessions with fewer
than messageLimit conversation messages.
- Around line 6114-6136: The usage listing still calls listAllSessions() with
default (heavy) scan budgets; update listSessionUsage to invoke the
lightweight/browse path by passing the browse flag or using the browse-specific
helper—e.g., call listAllSessions({ source, limit, forceRefresh:
!!params.forceRefresh, browse: true }) or replace with the equivalent
listAllSessionsBrowse(...) helper so the endpoint uses the new
lightweight/browse budgets (refer to listSessionUsage and listAllSessions in the
diff).
---
Nitpick comments:
In `@tests/unit/session-detail-preview-fast.test.mjs`:
- Around line 15-35: The test brittlely extracts internal functions via regex;
instead move the four functions applySessionDetailRecordMetadata,
extractSessionDetailPreviewFromTailText,
extractSessionDetailPreviewFromFileFast, and extractMessageFromRecord into a new
test helper module (e.g., tests/unit/session-detail-preview-helpers.js), export
them, update this test to import those functions directly, remove the
extractFunction/regex logic and the dynamic Function(...) wrapper, and adapt
instantiateExtractSessionDetailPreviewFromFileFast to return the imported
extractSessionDetailPreviewFromFileFast (wiring any required test bindings via
parameters or simple wrapper functions) so the test no longer depends on cli.js
source shape.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6608459e-a064-4f89-83b6-264f7486b4b2
📒 Files selected for processing (3)
cli.jstests/unit/run.mjstests/unit/session-detail-preview-fast.test.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/run.mjs
📜 Review details
🔇 Additional comments (3)
tests/unit/session-detail-preview-fast.test.mjs (1)
38-100: Good regression coverage for fast-tail preview semantics.The test data shape and assertions directly protect the intended contract (
tail window,clipped,non-exact totalMessages) and include reliable cleanup.cli.js (2)
5500-5575: Cache cloning looks good.Cloning on both store and read isolates cached session objects, so request-local mutations like
match.snippetswill not leak into later responses.
11138-11169: Nice readiness-probe split.Using a dedicated
health-checkaction here keeps browser auto-open side-effect free and avoids consuminginitNoticeduring startup.Also applies to: 11260-11263
|
@coderabbitai re-review |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
|
@coderabbitai re-review |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/unit/session-usage-backend.test.mjs (2)
157-190: Add lower-bound and non-numericlimitcases.This test already verifies default and upper clamp. Adding
limit <= 0and non-numeric input would fully pin the normalization behavior.Suggested test extension
test('listSessionUsage normalizes source and default limit for lightweight usage aggregation', async () => { @@ await listSessionUsage({ source: 'invalid', limit: '9999', forceRefresh: 1 }); + await listSessionUsage({ + source: 'codex', + limit: 0, + forceRefresh: 0 + }); + await listSessionUsage({ + source: 'claude', + limit: 'not-a-number' + }); await listSessionUsage({}); assert.deepStrictEqual(calls, [ @@ { + source: 'codex', + limit: 1, + forceRefresh: false + }, + { + source: 'claude', + limit: 200, + forceRefresh: false + }, + { source: 'all', limit: 200, forceRefresh: false } ]); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/session-usage-backend.test.mjs` around lines 157 - 190, Extend the existing test for listSessionUsage to also assert normalization when limit is non-positive or non-numeric: call listSessionUsage with limit: 0 and limit: -5 and with limit: 'abc' (and/or undefined numeric strings) and verify listSessionBrowse receives the clamped minimum/default (e.g., 1 or the function's lower bound) and correct forceRefresh coercion; update the expected calls array alongside the existing cases so instantiateListSessionUsage, listSessionBrowse, MAX_SESSION_LIST_SIZE and listSessionUsage are exercised for these new inputs and confirm behavior matches the normalization rules.
14-21: Make extraction less formatting-sensitive.Line 15’s regex-based function extraction is brittle; small formatting shifts in
cli.jscan break tests without behavior changes. Prefer a brace-balanced extractor (or AST parsing) for stability.Proposed refactor
function extractFunction(content, funcName) { - const regex = new RegExp(`(?:async\\s+)?function ${funcName}\\([^)]*\\)\\s*\\{[\\s\\S]*?^\\}`, 'm'); - const match = content.match(regex); - if (!match) { - throw new Error(`Function ${funcName} not found`); - } - return match[0]; + const signature = new RegExp(`(?:async\\s+)?function\\s+${funcName}\\s*\\(`); + const start = content.search(signature); + if (start === -1) { + throw new Error(`Function ${funcName} not found`); + } + const openBrace = content.indexOf('{', start); + if (openBrace === -1) { + throw new Error(`Function ${funcName} has no body`); + } + + let depth = 0; + for (let i = openBrace; i < content.length; i += 1) { + const ch = content[i]; + if (ch === '{') depth += 1; + if (ch === '}') depth -= 1; + if (depth === 0) { + return content.slice(start, i + 1); + } + } + throw new Error(`Function ${funcName} body is not balanced`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/session-usage-backend.test.mjs` around lines 14 - 21, The current extractFunction implementation using a fragile regex (in tests/unit/session-usage-backend.test.mjs: extractFunction and its regex) fails on minor formatting changes; replace it with a robust parser-based or brace-balanced extractor: parse the file into an AST (e.g., acorn/espree) and locate the FunctionDeclaration/FunctionExpression whose id.name equals funcName (or a top-level async function with that name), then return the original source slice using the node.start/node.end positions; alternatively implement a simple brace-matching routine that finds the function keyword for funcName and walks characters counting braces to locate the matching closing brace to extract the exact source.tests/e2e/test-web-ui-session-browser.js (1)
59-82: Consider adding error response handling capability to the mock fetch.The mock fetch always returns
ok: true, status: 200. While this works for the current happy-path tests, consider adding a mechanism to simulate error responses if you need to test error handling in the session browser.💡 Example extension for error simulation
function createApiFetch(api) { return async function fetch(_url, init = {}) { const payload = init && init.body ? JSON.parse(init.body) : {}; const result = await api(payload.action, payload.params); const raw = JSON.stringify(result || {}); + const hasError = result && result.error; return { - ok: true, - status: 200, + ok: !hasError, + status: hasError ? 400 : 200, headers: { get(name) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/test-web-ui-session-browser.js` around lines 59 - 82, The mock createApiFetch always returns a successful response (ok: true, status: 200), so tests cannot simulate HTTP errors; update createApiFetch to support error simulation by letting the provided api callback either throw or return an error-shaped result and then map that into a non-200 response (e.g., ok: false, status: <code>, and an error JSON body). Specifically, modify createApiFetch to detect when api(payload.action, payload.params) throws or returns an object with an error indicator (e.g., result.error or result.__status) and produce an appropriate response object (status, ok boolean, headers, and json/text that return the error payload) so tests can assert error handling in fetch consumers like the session browser.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/e2e/test-web-ui-session-browser.js`:
- Around line 59-82: The mock createApiFetch always returns a successful
response (ok: true, status: 200), so tests cannot simulate HTTP errors; update
createApiFetch to support error simulation by letting the provided api callback
either throw or return an error-shaped result and then map that into a non-200
response (e.g., ok: false, status: <code>, and an error JSON body).
Specifically, modify createApiFetch to detect when api(payload.action,
payload.params) throws or returns an object with an error indicator (e.g.,
result.error or result.__status) and produce an appropriate response object
(status, ok boolean, headers, and json/text that return the error payload) so
tests can assert error handling in fetch consumers like the session browser.
In `@tests/unit/session-usage-backend.test.mjs`:
- Around line 157-190: Extend the existing test for listSessionUsage to also
assert normalization when limit is non-positive or non-numeric: call
listSessionUsage with limit: 0 and limit: -5 and with limit: 'abc' (and/or
undefined numeric strings) and verify listSessionBrowse receives the clamped
minimum/default (e.g., 1 or the function's lower bound) and correct forceRefresh
coercion; update the expected calls array alongside the existing cases so
instantiateListSessionUsage, listSessionBrowse, MAX_SESSION_LIST_SIZE and
listSessionUsage are exercised for these new inputs and confirm behavior matches
the normalization rules.
- Around line 14-21: The current extractFunction implementation using a fragile
regex (in tests/unit/session-usage-backend.test.mjs: extractFunction and its
regex) fails on minor formatting changes; replace it with a robust parser-based
or brace-balanced extractor: parse the file into an AST (e.g., acorn/espree) and
locate the FunctionDeclaration/FunctionExpression whose id.name equals funcName
(or a top-level async function with that name), then return the original source
slice using the node.start/node.end positions; alternatively implement a simple
brace-matching routine that finds the function keyword for funcName and walks
characters counting braces to locate the matching closing brace to extract the
exact source.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9fd0661d-30f4-4e09-a07b-b13bc7ee8341
📒 Files selected for processing (7)
cli.jstests/e2e/test-sessions.jstests/e2e/test-web-ui-session-browser.jstests/unit/session-detail-preview-fast.test.mjstests/unit/session-usage-backend.test.mjstests/unit/web-run-host.test.mjsweb-ui/session-helpers.mjs
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/unit/web-run-host.test.mjs
- tests/unit/session-detail-preview-fast.test.mjs
- cli.js
📜 Review details
🔇 Additional comments (16)
tests/unit/session-usage-backend.test.mjs (1)
52-155: Strong regression coverage for lightweight and cache paths.The tests do a good job protecting the lightweight browse/usage behavior and cache-hit short-circuit logic (including sessionId → filePath lookup).
Also applies to: 192-452
web-ui/session-helpers.mjs (7)
14-27: LGTM!The option normalization helpers are well-structured. The
hasOwnOptionutility properly checks for explicit property presence, andnormalizeSessionLoadOptionscorrectly tracks whetherincludeActiveDetailwas explicitly provided viahasIncludeActiveDetail, allowing downstream code to apply contextual defaults.
30-46: LGTM!The merge logic in
mergeSessionLoadOptionscorrectly combines option sets:forceRefreshuses logical OR, andincludeActiveDetailis only true if either set explicitly specified it as true. TheshouldIncludeActiveSessionDetailfunction provides sensible contextual defaults based on tab state when no explicit value was provided.
52-55: Good use of a monotonic ticket to prevent stale hydration.The ticket pattern ensures that if multiple hydration tasks are scheduled in quick succession, only the latest one will execute. This prevents stale data from overwriting more recent results.
165-173: LGTM!When a load is already in progress, merging the new options into
__sessionPendingLoadOptionscorrectly coalesces multiple concurrent requests. This avoids redundant API calls while preserving intent from all callers.
258-263: Good fix: recursive call now outside thefinallyblock.Moving the recursive
loadSessionscall outside thefinallyblock ensures exceptions from thetryblock propagate correctly instead of being suppressed. The pending options are still captured and cleared withinfinallyfor proper cleanup.
369-383: LGTM!The sync task now has proper guards checking that the sessions tab is still active, preview rendering is enabled, and the
activeSessionhasn't changed since the task was scheduled. This prevents timeline sync from running in stale contexts.
460-462: LGTM!The condition change from
totalKnown > 0tototalKnown > totalcorrectly limits the next fetch boundary only when we know there are more messages on disk than currently loaded. This avoids incorrectly capping whentotalKnownis zero or equal tototal.tests/e2e/test-web-ui-session-browser.js (4)
150-160: LGTM!The guard limit of 50 iterations is a sensible safeguard to detect infinite loop scenarios in tests where scheduled frames keep spawning new frames. The error message clearly indicates the failure mode.
173-266: LGTM!The test setup is comprehensive: it creates isolated session fixtures with varying characteristics (dense list, huge-line, hot large sessions), initializes the VM with proper mocks, and exercises the session browser tab entry flow with appropriate assertions for list loading, detail hydration, and preview capping behavior.
268-295: LGTM!The hot session test correctly validates that huge sessions are clipped during preview, that only the initial batch is rendered, and that incremental loading properly expands the detail window without fetching the entire session at once.
297-314: LGTM!The huge-line session recovery test validates the fallback path when huge lines exceed the fast tail window, ensuring messages aren't duplicated and the preview is marked as fully recovered.
tests/e2e/test-sessions.js (4)
121-122: LGTM!New test fixture variables for the huge-line session are properly scoped within the test function, following the same pattern as other session fixtures.
269-270: LGTM!The relaxed assertions correctly accommodate the new preview behavior where
messageCountmay not be an exact value. Checking forNumber.isFiniteand>= 0ensures the field is still a valid numeric value without requiring exact equality.
293-300: LGTM!The preview mode assertions correctly validate that:
- Preview respects the
messageLimitparameter- Preview stays clipped for long sessions
- Preview intentionally avoids exact
totalMessagesto keep the operation lightweight
309-353: LGTM!Comprehensive test coverage for huge-line sessions:
- Preview mode validates fallback behavior, no message duplication, unclipped status for small sessions, and 4000-character text capping
- Non-preview mode validates exact
totalMessages, full message count, and preserved original huge content
|
@coderabbitai re-review |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
web-ui/app.js (1)
415-437: Extract the initial-load delay constant
120is duplicated in two scheduling branches, which makes tuning/error-prone later. Prefer a named constant (e.g.,INITIAL_LOAD_DELAY_MS) used in both places.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/app.js` around lines 415 - 437, The hard-coded delay 120ms is duplicated in runInitialLoad's scheduling branches; extract it into a single named constant (e.g., INITIAL_LOAD_DELAY_MS) and use that constant in both places where setTimeout is called (the branch inside requestAnimationFrame and the fallback setTimeout) so tuning is centralized; update references in runInitialLoad and ensure no other logic changes to this._initialLoadTimer or this._initialLoadRafId.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web-ui/modules/app.methods.session-timeline.mjs`:
- Around line 186-201: updateSessionTimelineOffset short-circuits when the
numeric offset hasn't changed, so if sessionPreviewContainerEl is replaced the
new DOM node never receives the CSS variable; fix by tracking the previous
container element and treating a container change as requiring re-apply: add a
private field (e.g. __sessionPreviewContainerEl) and compare it to the current
container (sessionPreviewContainerEl or this.$refs.sessionPreviewContainer) and
only early-return when both the offset and container reference are unchanged;
when container differs, set the CSS variable on the new container via
container.style.setProperty('--session-preview-header-offset', `${offset}px`)
and update both __sessionPreviewHeaderOffsetPx and __sessionPreviewContainerEl
accordingly (also ensure you remove property from the old container if needed).
---
Nitpick comments:
In `@web-ui/app.js`:
- Around line 415-437: The hard-coded delay 120ms is duplicated in
runInitialLoad's scheduling branches; extract it into a single named constant
(e.g., INITIAL_LOAD_DELAY_MS) and use that constant in both places where
setTimeout is called (the branch inside requestAnimationFrame and the fallback
setTimeout) so tuning is centralized; update references in runInitialLoad and
ensure no other logic changes to this._initialLoadTimer or
this._initialLoadRafId.
🪄 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: 98ad69a8-f494-442a-917d-a7f7fdbf1620
📒 Files selected for processing (7)
tests/unit/session-browser-timeline-regression.test.mjstests/unit/web-ui-behavior-parity.test.mjstests/unit/web-ui-runtime-navigation-regression.test.mjsweb-ui/app.jsweb-ui/logic.sessions.mjsweb-ui/modules/app.methods.navigation.mjsweb-ui/modules/app.methods.session-timeline.mjs
✅ Files skipped from review due to trivial changes (2)
- tests/unit/session-browser-timeline-regression.test.mjs
- web-ui/logic.sessions.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- web-ui/modules/app.methods.navigation.mjs
📜 Review details
🔇 Additional comments (2)
tests/unit/web-ui-behavior-parity.test.mjs (1)
1022-1039: Good regression assertion for pre-prime preview visibilityThe new check directly validates initial batching behavior (
sessionPreviewInitialBatchSize) instead of depending on HEAD parity for intentionally changed behavior.tests/unit/web-ui-runtime-navigation-regression.test.mjs (1)
404-441: Nice coverage for measured-element gating in viewport auto-growThis test captures the intended idle-fill guard and prevents regressions where visible count expands before a measurable list ref exists.
| updateSessionTimelineOffset() { | ||
| const container = this.sessionPreviewContainerEl || this.$refs.sessionPreviewContainer; | ||
| if (!container || !container.style) return; | ||
| if (!this.hasRenderableSessionTimeline()) { | ||
| if (this.__sessionPreviewHeaderOffsetPx != null) { | ||
| container.style.removeProperty('--session-preview-header-offset'); | ||
| this.__sessionPreviewHeaderOffsetPx = null; | ||
| } | ||
| return; | ||
| } | ||
| const offset = getSessionPreviewHeaderOffset(this); | ||
| if (this.__sessionPreviewHeaderOffsetPx === offset) { | ||
| return; | ||
| } | ||
| this.__sessionPreviewHeaderOffsetPx = offset; | ||
| container.style.setProperty('--session-preview-header-offset', `${offset}px`); |
There was a problem hiding this comment.
Cache short-circuit skips applying offset on container replacement
When sessionPreviewContainerEl changes (Line 35–37), the check on Line 197 can return early if the offset value is unchanged, so the new container never receives --session-preview-header-offset. That causes stale/fallback positioning on re-rendered containers.
Proposed fix
updateSessionTimelineOffset() {
const container = this.sessionPreviewContainerEl || this.$refs.sessionPreviewContainer;
if (!container || !container.style) return;
+ const containerChanged = this.__sessionPreviewOffsetContainer !== container;
if (!this.hasRenderableSessionTimeline()) {
- if (this.__sessionPreviewHeaderOffsetPx != null) {
+ if (containerChanged || this.__sessionPreviewHeaderOffsetPx != null) {
container.style.removeProperty('--session-preview-header-offset');
this.__sessionPreviewHeaderOffsetPx = null;
}
+ this.__sessionPreviewOffsetContainer = container;
return;
}
const offset = getSessionPreviewHeaderOffset(this);
- if (this.__sessionPreviewHeaderOffsetPx === offset) {
+ if (!containerChanged && this.__sessionPreviewHeaderOffsetPx === offset) {
return;
}
this.__sessionPreviewHeaderOffsetPx = offset;
+ this.__sessionPreviewOffsetContainer = container;
container.style.setProperty('--session-preview-header-offset', `${offset}px`);
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-ui/modules/app.methods.session-timeline.mjs` around lines 186 - 201,
updateSessionTimelineOffset short-circuits when the numeric offset hasn't
changed, so if sessionPreviewContainerEl is replaced the new DOM node never
receives the CSS variable; fix by tracking the previous container element and
treating a container change as requiring re-apply: add a private field (e.g.
__sessionPreviewContainerEl) and compare it to the current container
(sessionPreviewContainerEl or this.$refs.sessionPreviewContainer) and only
early-return when both the offset and container reference are unchanged; when
container differs, set the CSS variable on the new container via
container.style.setProperty('--session-preview-header-offset', `${offset}px`)
and update both __sessionPreviewHeaderOffsetPx and __sessionPreviewContainerEl
accordingly (also ensure you remove property from the old container if needed).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web-ui/modules/app.methods.session-browser.mjs`:
- Around line 8-15: The helper isSessionLoadNativeDialogEnabled can re-enter
itself when vm.isSessionLoadNativeDialogEnabled is a wrapper that calls back
into the helper; add a reentrancy guard on the vm to prevent recursion: define a
unique Symbol (e.g., const SESSION_PROBE_LOCK = Symbol.for('sessionProbeLock'))
and, before invoking vm.isSessionLoadNativeDialogEnabled(), check if
vm[SESSION_PROBE_LOCK] is truthy and return false if so; otherwise set
vm[SESSION_PROBE_LOCK] = true, call vm.isSessionLoadNativeDialogEnabled(), and
clear vm[SESSION_PROBE_LOCK] in a finally block to ensure the flag is removed
after the probe; apply this guard in isSessionLoadNativeDialogEnabled and any
wrapper that calls the helper (references: isSessionLoadNativeDialogEnabled and
vm.isSessionLoadNativeDialogEnabled).
🪄 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: 921ee8bd-209a-49ff-a296-fea9bf29bec3
📒 Files selected for processing (3)
tests/unit/web-ui-behavior-parity.test.mjsweb-ui/modules/app.methods.session-browser.mjsweb-ui/session-helpers.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.352Z
Learning: In PR `#55` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-04-04T01:36:26.964Z
Learning: In PR `#68` for SakuraByteCore/codexmate, github-actions[bot] again flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint (reinforced across PRs `#55`, `#59`, `#67`, `#68`) is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-31T12:37:51.023Z
Learning: In PR `#59` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-04-03T10:42:22.368Z
Learning: In PR `#67` for SakuraByteCore/codexmate, github-actions[bot] flagged that CodeRabbit was again introducing breaking changes. The maintainer's absolute constraint (reinforced across PRs `#55`, `#59`, `#67`) is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: SurviveM
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-29T02:42:32.785Z
Learning: The repository maintainer SurviveM (GitHub: SurviveM, repo: SakuraByteCore/codexmate) prefers that CodeRabbit review suggestions do not introduce breaking changes. Reviews should focus on correctness, safety, and quality without recommending refactors or changes that would break existing APIs, interfaces, or behavior.
| function isSessionLoadNativeDialogEnabled(vm) { | ||
| if (vm && typeof vm.isSessionLoadNativeDialogEnabled === 'function' && vm.isSessionLoadNativeDialogEnabled !== isSessionLoadNativeDialogEnabled) { | ||
| try { | ||
| return !!vm.isSessionLoadNativeDialogEnabled(); | ||
| } catch (_) { | ||
| // fall through to shared detection | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid the self-recursive native-dialog probe.
With the wrapper at Lines 150-152, the helper now calls back into vm.isSessionLoadNativeDialogEnabled(), which re-enters the same helper and recurses until a RangeError is thrown and caught. Every session-load debug probe goes through that path, so this adds a stack overflow to normal control flow.
🐛 Minimal fix
function isSessionLoadNativeDialogEnabled(vm) {
- if (vm && typeof vm.isSessionLoadNativeDialogEnabled === 'function' && vm.isSessionLoadNativeDialogEnabled !== isSessionLoadNativeDialogEnabled) {
- try {
- return !!vm.isSessionLoadNativeDialogEnabled();
- } catch (_) {
- // fall through to shared detection
- }
- }
-
try {
if (globalThis && globalThis.__CODEXMATE_SESSION_LOAD_NATIVE_DIALOG__ === true) {
return true;
}Also applies to: 150-152
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-ui/modules/app.methods.session-browser.mjs` around lines 8 - 15, The
helper isSessionLoadNativeDialogEnabled can re-enter itself when
vm.isSessionLoadNativeDialogEnabled is a wrapper that calls back into the
helper; add a reentrancy guard on the vm to prevent recursion: define a unique
Symbol (e.g., const SESSION_PROBE_LOCK = Symbol.for('sessionProbeLock')) and,
before invoking vm.isSessionLoadNativeDialogEnabled(), check if
vm[SESSION_PROBE_LOCK] is truthy and return false if so; otherwise set
vm[SESSION_PROBE_LOCK] = true, call vm.isSessionLoadNativeDialogEnabled(), and
clear vm[SESSION_PROBE_LOCK] in a finally block to ensure the flag is removed
after the probe; apply this guard in isSessionLoadNativeDialogEnabled and any
wrapper that calls the helper (references: isSessionLoadNativeDialogEnabled and
vm.isSessionLoadNativeDialogEnabled).
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
cli.js (1)
4784-4801:⚠️ Potential issue | 🟠 MajorUse a real exact pass after the last chunk has been read.
When
positionreaches0,combinedalready contains the whole file, but this code still returns or derives from the tail-limitedlateststate. That leaves preview mode inexact for 256 KB–1 MB sessions with more thanmessageLimitmessages: today it returnstotalMessages: null, and even without the early return the post-loop block would still compute totals from a truncated slice.🩹 Minimal fix
totalBytesRead += bytesRead; combined = Buffer.concat([chunk.subarray(0, bytesRead), combined]); latest = extractSessionDetailPreviewFromTailText(combined.toString('utf-8'), source, safeMessageLimit); - if (latest.messages.length >= safeMessageLimit) { + if (latest.messages.length >= safeMessageLimit && position > 0) { latest.clipped = latest.clipped || position > 0; return latest; } } if (position > 0) { latest.clipped = latest.clipped || position > 0; return latest; } - const normalizedMessages = removeLeadingSystemMessage(latest.messages); - latest.messages = normalizedMessages.length > safeMessageLimit - ? normalizedMessages.slice(-safeMessageLimit) - : normalizedMessages; - latest.totalMessages = normalizedMessages.length; - latest.clipped = latest.totalMessages > latest.messages.length; - return latest; + const exact = extractSessionDetailPreviewFromRecords( + parseJsonlContent(combined.toString('utf-8')), + source, + safeMessageLimit + ); + exact.clipped = exact.totalMessages > exact.messages.length; + return exact;Based on learnings the maintainer prefers non-breaking fixes only, so this keeps the current preview contract and only corrects the full-file fast-preview case.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 4784 - 4801, When position reaches 0 we must perform an exact pass over the full file instead of returning the tail-limited preview: call extractSessionDetailPreviewFromTailText(combined.toString('utf-8'), source, safeMessageLimit) again to obtain a fresh exact latest, then run removeLeadingSystemMessage(latest.messages) and recompute latest.messages (slicing to safeMessageLimit), latest.totalMessages and latest.clipped from that normalized array; replace the current early/late returns that use a possibly truncated latest when position === 0 so totals and clipped reflect the full content.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e/test-sessions.js`:
- Around line 124-125: The huge-line fixture file created via hugeLineSessionId
/ hugeLineSessionPath is not removed in the test's finally cleanup and can leak
state; add a best-effort deletion in the finally block that mirrors other
cleanup (call bestEffortApi('delete-session', { source: 'codex', sessionId:
hugeLineSessionId, filePath: hugeLineSessionPath }) or otherwise unlink
hugeLineSessionPath) so the synthetic session file is removed after the test
completes.
In `@web-ui/modules/app.computed.session.mjs`:
- Around line 89-91: The current computation of initialBatchSize calls
Number.isFinite on the raw this.sessionPreviewInitialBatchSize, so numeric
strings like "24" fall back to 12; normalize the value first by coercing to a
number (e.g., const raw = Number(this.sessionPreviewInitialBatchSize) or
parseFloat) then check Number.isFinite(raw) and use Math.max(1, Math.floor(raw))
when valid, otherwise use 12—update the logic around
initialBatchSize/sessionPreviewInitialBatchSize accordingly.
- Around line 67-70: The findIndex callback calls
this.getSessionExportKey(session) without guarding against null/undefined
entries in the list (e.g., from sortedSessionsList), which can throw; update the
logic that computes activeIndex (or sanitize the list first) so you either
filter out falsy sessions from list before calling findIndex, or change the
callback to first check session truthiness (e.g., session &&
this.getSessionExportKey(session) === activeKey) to safely skip null entries;
refer to this.activeSession, getSessionExportKey, and the local variable list
when making the change.
---
Duplicate comments:
In `@cli.js`:
- Around line 4784-4801: When position reaches 0 we must perform an exact pass
over the full file instead of returning the tail-limited preview: call
extractSessionDetailPreviewFromTailText(combined.toString('utf-8'), source,
safeMessageLimit) again to obtain a fresh exact latest, then run
removeLeadingSystemMessage(latest.messages) and recompute latest.messages
(slicing to safeMessageLimit), latest.totalMessages and latest.clipped from that
normalized array; replace the current early/late returns that use a possibly
truncated latest when position === 0 so totals and clipped reflect the full
content.
🪄 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: 7e880ad8-beb2-4440-b017-2a05e248db27
📒 Files selected for processing (7)
cli.jstests/e2e/test-sessions.jstests/e2e/test-setup.jstests/unit/session-usage-backend.test.mjstests/unit/session-usage.test.mjsweb-ui/logic.sessions.mjsweb-ui/modules/app.computed.session.mjs
✅ Files skipped from review due to trivial changes (2)
- tests/e2e/test-setup.js
- tests/unit/session-usage-backend.test.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- web-ui/logic.sessions.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.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.
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: SurviveM
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-29T02:42:32.785Z
Learning: The repository maintainer SurviveM (GitHub: SurviveM, repo: SakuraByteCore/codexmate) prefers that CodeRabbit review suggestions do not introduce breaking changes. Reviews should focus on correctness, safety, and quality without recommending refactors or changes that would break existing APIs, interfaces, or behavior.
🔇 Additional comments (7)
web-ui/modules/app.computed.session.mjs (1)
8-14: Good defensive formatting and summary-card extension.The added number sanitizer plus
totalTokens/totalContextWindowcards are consistent and keep invalid values from leaking into UI stats.Also applies to: 150-156
tests/unit/session-usage.test.mjs (1)
13-15: Nice regression coverage for new usage totals.These assertions correctly pin both positive aggregation and invalid-session zeroing for
totalTokensandtotalContextWindow.Also applies to: 20-21, 49-49, 54-55
cli.js (5)
125-137: LGTM: the lightweight cache layers are cleanly separated.Inventory cloning plus the separate
sessionId -> filePathlookup cache avoids caller-mutation leakage while still short-circuiting repeated path scans.Also applies to: 515-519, 5468-5607, 5610-5616, 6294-6353
5618-5712: LGTM: usage loading now stays on summary metadata.Pulling
totalTokensandcontextWindowinto the summary readers letslistSessionUsage()stay on the lightweight browse path without changing its response shape.Also applies to: 5714-6127, 6231-6270
4680-4742: LGTM: preview-mode response shaping is explicit.Preview-only text capping and nullable
totalMessagesmake the preview/detail contract straightforward for the UI.Also applies to: 4811-4817, 8095-8127
11280-11414: LGTM: the readiness probe is now side-effect-free.
health-checkno longer consumes init state, and browser auto-open waits until the local API is actually responding.Also applies to: 11975-11977
6142-6170: LGTM: the lightweight browse path is wired consistently across surfaces.HTTP session listing, workflows, MCP tools, and MCP resources all route through
listSessionBrowse(), so the cheaper browse behavior stays aligned everywhere.Also applies to: 11642-11644, 13356-13360, 13725-13727, 14138-14145
| const hugeLineSessionId = 'codex-huge-line-preview-e2e'; | ||
| const hugeLineSessionPath = path.join(tmpHome, '.codex', 'sessions', `${hugeLineSessionId}.jsonl`); |
There was a problem hiding this comment.
Clean up the huge-line fixture in finally.
hugeLineSessionPath is created on Lines 312-328, but it is never removed in the cleanup block. Unlike longSessionId, this synthetic session is not carried forward via ctx, so a reused tmpHome can leak extra state into later session-list/usage assertions.
Minimal cleanup
await bestEffortApi('delete-session', {
source: 'codex',
sessionId: hugeLineSessionId,
filePath: hugeLineSessionPath
});Also applies to: 312-328
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/test-sessions.js` around lines 124 - 125, The huge-line fixture
file created via hugeLineSessionId / hugeLineSessionPath is not removed in the
test's finally cleanup and can leak state; add a best-effort deletion in the
finally block that mirrors other cleanup (call bestEffortApi('delete-session', {
source: 'codex', sessionId: hugeLineSessionId, filePath: hugeLineSessionPath })
or otherwise unlink hugeLineSessionPath) so the synthetic session file is
removed after the test completes.
| const activeKey = this.activeSession ? this.getSessionExportKey(this.activeSession) : ''; | ||
| if (activeKey) { | ||
| const activeIndex = list.findIndex((session) => this.getSessionExportKey(session) === activeKey); | ||
| if (activeIndex >= 0) { |
There was a problem hiding this comment.
Guard session lookup against null entries in visibleSessionsList.
Line 69 calls this.getSessionExportKey(session) without a null check. If sortedSessionsList includes a falsy item, this can throw during render when activeKey is set.
Proposed non-breaking fix
- if (activeKey) {
- const activeIndex = list.findIndex((session) => this.getSessionExportKey(session) === activeKey);
+ if (activeKey) {
+ const activeIndex = list.findIndex((session) => session && this.getSessionExportKey(session) === activeKey);
if (activeIndex >= 0) {
targetCount = Math.max(targetCount, activeIndex + 1);
}
}📝 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 activeKey = this.activeSession ? this.getSessionExportKey(this.activeSession) : ''; | |
| if (activeKey) { | |
| const activeIndex = list.findIndex((session) => this.getSessionExportKey(session) === activeKey); | |
| if (activeIndex >= 0) { | |
| const activeKey = this.activeSession ? this.getSessionExportKey(this.activeSession) : ''; | |
| if (activeKey) { | |
| const activeIndex = list.findIndex((session) => session && this.getSessionExportKey(session) === activeKey); | |
| if (activeIndex >= 0) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-ui/modules/app.computed.session.mjs` around lines 67 - 70, The findIndex
callback calls this.getSessionExportKey(session) without guarding against
null/undefined entries in the list (e.g., from sortedSessionsList), which can
throw; update the logic that computes activeIndex (or sanitize the list first)
so you either filter out falsy sessions from list before calling findIndex, or
change the callback to first check session truthiness (e.g., session &&
this.getSessionExportKey(session) === activeKey) to safely skip null entries;
refer to this.activeSession, getSessionExportKey, and the local variable list
when making the change.
| const initialBatchSize = Number.isFinite(this.sessionPreviewInitialBatchSize) | ||
| ? Math.max(1, Math.floor(this.sessionPreviewInitialBatchSize)) | ||
| : 12; |
There was a problem hiding this comment.
Normalize sessionPreviewInitialBatchSize before finite check.
Line 89 checks finiteness on the raw value. Numeric strings (e.g. "24") are treated as invalid and silently fall back to 12 instead of the configured batch size.
Proposed non-breaking fix
- const initialBatchSize = Number.isFinite(this.sessionPreviewInitialBatchSize)
- ? Math.max(1, Math.floor(this.sessionPreviewInitialBatchSize))
+ const rawInitialBatchSize = Number(this.sessionPreviewInitialBatchSize);
+ const initialBatchSize = Number.isFinite(rawInitialBatchSize)
+ ? Math.max(1, Math.floor(rawInitialBatchSize))
: 12;📝 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 initialBatchSize = Number.isFinite(this.sessionPreviewInitialBatchSize) | |
| ? Math.max(1, Math.floor(this.sessionPreviewInitialBatchSize)) | |
| : 12; | |
| const rawInitialBatchSize = Number(this.sessionPreviewInitialBatchSize); | |
| const initialBatchSize = Number.isFinite(rawInitialBatchSize) | |
| ? Math.max(1, Math.floor(rawInitialBatchSize)) | |
| : 12; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-ui/modules/app.computed.session.mjs` around lines 89 - 91, The current
computation of initialBatchSize calls Number.isFinite on the raw
this.sessionPreviewInitialBatchSize, so numeric strings like "24" fall back to
12; normalize the value first by coercing to a number (e.g., const raw =
Number(this.sessionPreviewInitialBatchSize) or parseFloat) then check
Number.isFinite(raw) and use Math.max(1, Math.floor(raw)) when valid, otherwise
use 12—update the logic around initialBatchSize/sessionPreviewInitialBatchSize
accordingly.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
web-ui/modules/app.computed.session.mjs (2)
101-103:⚠️ Potential issue | 🟡 MinorCoerce
sessionPreviewInitialBatchSizebefore validating it.Lines 101-103 still use
Number.isFinite(this.sessionPreviewInitialBatchSize), so numeric strings like"24"are treated as invalid and silently fall back to12.Non-breaking fix
- const initialBatchSize = Number.isFinite(this.sessionPreviewInitialBatchSize) - ? Math.max(1, Math.floor(this.sessionPreviewInitialBatchSize)) + const rawInitialBatchSize = Number(this.sessionPreviewInitialBatchSize); + const initialBatchSize = Number.isFinite(rawInitialBatchSize) + ? Math.max(1, Math.floor(rawInitialBatchSize)) : 12;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/modules/app.computed.session.mjs` around lines 101 - 103, The code treats numeric strings as invalid because it checks Number.isFinite(this.sessionPreviewInitialBatchSize) without coercion; change the logic that computes initialBatchSize to coerce sessionPreviewInitialBatchSize to a Number first (e.g., const value = Number(this.sessionPreviewInitialBatchSize)), then validate with Number.isFinite(value) and compute Math.max(1, Math.floor(value)) when valid, otherwise fall back to 12; update the expressions referencing sessionPreviewInitialBatchSize/initialBatchSize accordingly to use the coerced value.
80-83:⚠️ Potential issue | 🟠 MajorGuard the active-session lookup against falsy entries.
Line 81 still calls
this.getSessionExportKey(session)unconditionally. IfsortedSessionsListcontains anull/undefineditem, this can still throw during render as soon asactiveKeyis set.Non-breaking fix
- if (activeKey) { - const activeIndex = list.findIndex((session) => this.getSessionExportKey(session) === activeKey); + if (activeKey) { + const activeIndex = list.findIndex((session) => session && this.getSessionExportKey(session) === activeKey); if (activeIndex >= 0) { targetCount = Math.max(targetCount, activeIndex + 1); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/modules/app.computed.session.mjs` around lines 80 - 83, The active-session lookup calls this.getSessionExportKey(session) unguarded and can throw if the sessions list contains null/undefined; update the findIndex predicate used when computing activeIndex (the list.findIndex callback that references this.getSessionExportKey(session)) to first check session != null (or session !== undefined) before calling this.getSessionExportKey, e.g. guard the call with a null-check or optional chaining so the predicate safely returns false for falsy entries and prevents runtime errors during render.
🤖 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/app.computed.session.mjs`:
- Around line 101-103: The code treats numeric strings as invalid because it
checks Number.isFinite(this.sessionPreviewInitialBatchSize) without coercion;
change the logic that computes initialBatchSize to coerce
sessionPreviewInitialBatchSize to a Number first (e.g., const value =
Number(this.sessionPreviewInitialBatchSize)), then validate with
Number.isFinite(value) and compute Math.max(1, Math.floor(value)) when valid,
otherwise fall back to 12; update the expressions referencing
sessionPreviewInitialBatchSize/initialBatchSize accordingly to use the coerced
value.
- Around line 80-83: The active-session lookup calls
this.getSessionExportKey(session) unguarded and can throw if the sessions list
contains null/undefined; update the findIndex predicate used when computing
activeIndex (the list.findIndex callback that references
this.getSessionExportKey(session)) to first check session != null (or session
!== undefined) before calling this.getSessionExportKey, e.g. guard the call with
a null-check or optional chaining so the predicate safely returns false for
falsy entries and prevents runtime errors during render.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 71123cfc-8d87-4fe3-924f-c8ca77a73add
📒 Files selected for processing (4)
tests/unit/config-tabs-ui.test.mjstests/unit/web-ui-logic.test.mjsweb-ui/modules/app.computed.session.mjsweb-ui/partials/index/panel-usage.html
🚧 Files skipped from review as they are similar to previous changes (2)
- web-ui/partials/index/panel-usage.html
- tests/unit/web-ui-logic.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-03-30T04:47:15.352Z
Learning: In PR `#55` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-04-04T01:36:26.964Z
Learning: In PR `#68` for SakuraByteCore/codexmate, github-actions[bot] again flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint (reinforced across PRs `#55`, `#59`, `#67`, `#68`) is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-31T12:37:51.023Z
Learning: In PR `#59` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-04-03T10:42:22.368Z
Learning: In PR `#67` for SakuraByteCore/codexmate, github-actions[bot] flagged that CodeRabbit was again introducing breaking changes. The maintainer's absolute constraint (reinforced across PRs `#55`, `#59`, `#67`) is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: SurviveM
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-29T02:42:32.785Z
Learning: The repository maintainer SurviveM (GitHub: SurviveM, repo: SakuraByteCore/codexmate) prefers that CodeRabbit review suggestions do not introduce breaking changes. Reviews should focus on correctness, safety, and quality without recommending refactors or changes that would break existing APIs, interfaces, or behavior.
3 participants
Summary
Tests
Summary by CodeRabbit
New Features
Reliability
Performance
UI/UX
Tests