Chat | Allow new conversations while a stream is active - #48
Conversation
New chat was waiting on Octavus session create, which could stall behind an in-flight trigger. Switch the UI optimistically and create sessions on a dedicated HTTP pool so parallel conversations stay usable. Co-authored-by: Cursor <cursoragent@cursor.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThe server now creates Octavus agent sessions through a dedicated Undici helper. Session-file updates execute in sequence across creation, resume, deletion, forking, and saving. The client displays new chats immediately as pending sessions. Sends and uploads wait for session creation before attaching the chat runtime. Pending-session deletion and creation failures clean up local and remote state. Tests cover API requests, serialized writes, server persistence, and delayed UI session creation. Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
public/app.js (1)
1518-1526: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThe composer is cleared before creation is resolved, so a failed create discards the user's message.
Lines 1518-1521 clear
promptInputand the attachment list. Line 1524 then awaitsensureActiveReady(). If creation fails,ensureActiveReady()returnsnull, and line 1525 returns without sending. The typed text and the ready file refs are already gone, and no error is shown to the user.Before this change,
isComposerSendAllowed()requiredactive.chat, so send never started without a runtime. Line 1706 removes that guarantee. Restore the composer content when the send does not start.🐛 Proposed fix
promptInput.value = ''; const filesToSend = readyRefs; clearAttachment(); updateSendBtn(); try { const rt = await ensureActiveReady(); - if (!rt?.chat) return; + if (!rt?.chat) { + // Creation failed or the thread was abandoned. Give the input back. + promptInput.value = text; + updateSendBtn(); + announceChatStatus(t('Could not start the conversation. Try again.')); + return; + } await rt.chat.send(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@public/app.js` around lines 1518 - 1526, Restore the composer state when sending cannot start: in the send flow around ensureActiveReady, preserve the original prompt text and readyRefs before clearing them, then restore the prompt and attachment references when ensureActiveReady returns no chat or creation fails. Ensure failed runtime creation does not discard the user’s message or files, while retaining the existing clear behavior after a send successfully starts.
🧹 Nitpick comments (5)
tests/server.test.js (1)
228-254: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the creation-failure path.
The suite covers only the success path.
createAgentSessionnow throws on non-OK responses and on a missingsessionId. The client inpublic/app.jsdepends on a non-2xx response to trigger the pending-thread rollback at lines 2040-2058. Assert thatPOST /api/sessionsreturns an error status when the helper rejects, and that nothing is written to the sessions file.🧪 Proposed test
+ it('returns an error status and persists nothing when create fails', async () => { + mockSessionsFile({ sessions: [] }); + createAgentSession.mockRejectedValue(new Error('upstream down')); + + const res = await request(app).post('/api/sessions').send({}); + + expect(res.status).toBeGreaterThanOrEqual(500); + expect(fs.writeFile).not.toHaveBeenCalled(); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/server.test.js` around lines 228 - 254, Add a failure-path test in the POST /api/sessions suite that makes createAgentSession reject, then assert the endpoint returns a non-2xx error response and fs.write is not called, confirming no session record is persisted when creation fails.tests/octavus-create.test.js (1)
48-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the missing
sessionIdbranch.
createAgentSessionrejects when the API returns 200 with no stringsessionId(lib/octavus-create.js lines 47-50). No test exercises that branch. This is the validation that protectsbuildSessionRecordin server.js from an undefined session id.🧪 Proposed test
+ it('throws when the API returns no sessionId', async () => { + fetchMock.mockResolvedValue({ + ok: true, + json: async () => ({}), + }); + + await expect( + createAgentSession({ baseUrl: 'https://octavus.example', agentId: 'agent-1' }), + ).rejects.toThrow(/sessionId/); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/octavus-create.test.js` around lines 48 - 68, Add a test alongside the existing createAgentSession rejection cases that mocks a successful 200 API response without a string sessionId and asserts createAgentSession rejects. Use the existing fetchMock and request options, and verify the rejection covers the validation error protecting buildSessionRecord.server.js (1)
139-147: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the
falsesentinel in the updater contract.
updateSessionsFilesupports three distinct mutator return shapes: a new object,undefined(write the mutateddata), andfalse(skip the write). No caller in this diff usesfalse, and the JSDoc does not describe it. A future caller that returns a falsy value by accident will silently skip persistence. Document the contract, or drop the sentinel until a caller needs it.📝 Proposed documentation
-/** Run a read-modify-write against chat-sessions.json without overlapping writers. */ +/** + * Run a read-modify-write against chat-sessions.json without overlapping writers. + * + * `@param` {(data: object) => object|false|void|Promise<object|false|void>} mutator + * Return the next state to persist, `undefined` to persist the mutated input, + * or `false` to skip the write. + * `@returns` {Promise<object>} The persisted (or unchanged) sessions data. + */ function updateSessionsFile(mutator) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server.js` around lines 139 - 147, Update the JSDoc for updateSessionsFile to document the mutator return contract: returning a new object writes it, returning undefined writes the existing mutated data, and returning false skips persistence. Keep the current false-sentinel behavior unchanged.tests/dom/render.test.js (1)
462-468: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the pending id is replaced by the real session id.
The test confirms the thread count is stable, but it does not confirm the id swap at public/app.js lines 2018-2030. A regression that leaves the
pending-*id inallSessionsMetastill passes. That id is the value the delete path at line 1879 uses to decide whether to call the server, so the swap is the important post-condition.Consider also adding a case for the failure path, where
POST /api/sessionsreturns a non-OK status. That case exercises the rollback at lines 2040-2058.🧪 Proposed assertion
// Create finished → runtime wired with a real chat instance. expect(fakeChats.length).toBe(chatsBefore + 1); expect(qa('.session-item').length).toBe(before + 1); + // The optimistic id is replaced by the real one, so delete hits the server. + q('.session-item .session-item__delete').click(); + await settle(); + expect(requests.some((r) => r.url === '/api/sessions/session-2' && r.method === 'DELETE')).toBe(true);Destructure
requestsfrombootApp()at line 448 to use this assertion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/dom/render.test.js` around lines 462 - 468, Extend the test around releaseNewSession to assert that allSessionsMeta no longer contains the pending-* id and instead contains the real session id returned by the session-creation request, using the requests captured from bootApp(). Add a failure-path case where POST /api/sessions returns a non-OK response and verify the pending session metadata is rolled back.lib/octavus-create.js (1)
12-38: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd an explicit timeout to session creation.
Undici 8.10.0 defaults
headersTimeoutandbodyTimeoutto 300,000 ms. Configure a shorter timeout, such as 15,000 ms, oncreateAgent. UseAbortSignal.timeout(15_000)if the request requires a total deadline.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/octavus-create.js` around lines 12 - 38, Configure createAgent with a 15,000 ms headersTimeout and bodyTimeout for session creation, or apply AbortSignal.timeout(15_000) to the undiciFetch request if a total request deadline is required. Keep the existing dispatcher usage in createAgentSession unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@public/app.js`:
- Around line 2040-2060: Update the startNewChat() catch cleanup to return null
instead of rethrowing, so callers receive a resolved failure result after
cleanup. In the active-session fallback, attach handling to the switchSession()
promise to prevent unhandled rejections while preserving the best-effort
behavior; keep the existing cleanup and rendering paths unchanged.
---
Outside diff comments:
In `@public/app.js`:
- Around line 1518-1526: Restore the composer state when sending cannot start:
in the send flow around ensureActiveReady, preserve the original prompt text and
readyRefs before clearing them, then restore the prompt and attachment
references when ensureActiveReady returns no chat or creation fails. Ensure
failed runtime creation does not discard the user’s message or files, while
retaining the existing clear behavior after a send successfully starts.
---
Nitpick comments:
In `@lib/octavus-create.js`:
- Around line 12-38: Configure createAgent with a 15,000 ms headersTimeout and
bodyTimeout for session creation, or apply AbortSignal.timeout(15_000) to the
undiciFetch request if a total request deadline is required. Keep the existing
dispatcher usage in createAgentSession unchanged.
In `@server.js`:
- Around line 139-147: Update the JSDoc for updateSessionsFile to document the
mutator return contract: returning a new object writes it, returning undefined
writes the existing mutated data, and returning false skips persistence. Keep
the current false-sentinel behavior unchanged.
In `@tests/dom/render.test.js`:
- Around line 462-468: Extend the test around releaseNewSession to assert that
allSessionsMeta no longer contains the pending-* id and instead contains the
real session id returned by the session-creation request, using the requests
captured from bootApp(). Add a failure-path case where POST /api/sessions
returns a non-OK response and verify the pending session metadata is rolled
back.
In `@tests/octavus-create.test.js`:
- Around line 48-68: Add a test alongside the existing createAgentSession
rejection cases that mocks a successful 200 API response without a string
sessionId and asserts createAgentSession rejects. Use the existing fetchMock and
request options, and verify the rejection covers the validation error protecting
buildSessionRecord.
In `@tests/server.test.js`:
- Around line 228-254: Add a failure-path test in the POST /api/sessions suite
that makes createAgentSession reject, then assert the endpoint returns a non-2xx
error response and fs.write is not called, confirming no session record is
persisted when creation fails.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cb09b582-9312-4a75-bc15-0654b914c5ad
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
lib/octavus-create.jslib/sessions-file.jspackage.jsonpublic/app.jsserver.jstests/dom/harness.jstests/dom/render.test.jstests/octavus-create.test.jstests/server.test.jstests/sessions-file.test.js
Restore composer content when session create fails before send, settle createPromise to null instead of rejecting, and cover the review nits with timeouts plus create-failure tests. Co-authored-by: Cursor <cursoragent@cursor.com>
1 participant
Summary
Starting a second conversation while another reply was still streaming felt blocked: New chat waited on
octavus.agentSessions.create, and that call could stall until the open trigger finished. Clicked threads then appeared in a burst when the stream ended.This restores the multiplex intent: you can open and use another conversation while one is still responding.
Changes
New chat switches the UI immediately with a local pending thread, then finishes Octavus create in the background. Send/upload on that thread wait for the real session id before talking to Octavus.
Session create now goes through a dedicated undici Agent (
lib/octavus-create.js) so a long-lived/api/triggerfetch on the default dispatcher cannot starve create. Worth a close look if you know Octavus connection behavior.chat-sessions.jsonupdates are queued (lib/sessions-file.js) so overlapping create/save/delete/fork writers cannot clobber each other under parallel streams.Test plan
npm test