Fix UI stuck on Stop when request is slow or proxy disconnects#995
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR makes SSE termination abort-aware across shared utilities and API clients, updates disconnected-port error handling, and tracks proxy/session port lifecycle state in the background and conversation card flows. ChangesAbort-aware stream termination and cleanup
Disconnected port error handling
Proxy and conversation port lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves stream abort handling across various API clients and the fetchSSE utility by passing an aborted flag to the onEnd callback and tracking the port's generation state. Feedback focuses on ensuring robust error handling by wrapping port.postMessage calls in try...catch blocks to prevent crashes when ports disconnect, maintaining consistency across all API clients by persisting partial answers on abort, using optional chaining for safer message property access, and explicitly passing false to onEnd upon normal completion.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| async onEnd(aborted) { | ||
| try { | ||
| if (!aborted) { | ||
| port.postMessage({ done: true }) | ||
| } | ||
| } finally { | ||
| port.onMessage.removeListener(messageListener) | ||
| port.onDisconnect.removeListener(disconnectListener) | ||
| } | ||
| }, |
There was a problem hiding this comment.
To maintain consistency with openai-compatible-core.mjs, we should also persist the streamed partial answer into the session history when the request is aborted. We should wrap the port.postMessage call in a try...catch block in case the port is disconnected.
async onEnd(aborted) {
try {
if (!aborted) {
port.postMessage({ done: true })
} else if (answer) {
pushRecord(session, question, answer)
try {
port.postMessage({ session })
} catch (e) {
console.warn('[azure-openai-api] Failed to post session on abort:', e)
}
}
} finally {
port.onMessage.removeListener(messageListener)
port.onDisconnect.removeListener(disconnectListener)
}
},| async onEnd(aborted) { | ||
| try { | ||
| if (!aborted) { | ||
| port.postMessage({ done: true }) | ||
| } | ||
| } finally { | ||
| port.onMessage.removeListener(messageListener) | ||
| port.onDisconnect.removeListener(disconnectListener) | ||
| } | ||
| }, |
There was a problem hiding this comment.
To maintain consistency with openai-compatible-core.mjs, we should also persist the streamed partial answer into the session history when the request is aborted. We should wrap the port.postMessage call in a try...catch block in case the port is disconnected.
| async onEnd(aborted) { | |
| try { | |
| if (!aborted) { | |
| port.postMessage({ done: true }) | |
| } | |
| } finally { | |
| port.onMessage.removeListener(messageListener) | |
| port.onDisconnect.removeListener(disconnectListener) | |
| } | |
| }, | |
| async onEnd(aborted) { | |
| try { | |
| if (!aborted) { | |
| port.postMessage({ done: true }) | |
| } else if (answer) { | |
| pushRecord(session, question, answer) | |
| try { | |
| port.postMessage({ session }) | |
| } catch (e) { | |
| console.warn('[claude-api] Failed to post session on abort:', e) | |
| } | |
| } | |
| } finally { | |
| port.onMessage.removeListener(messageListener) | |
| port.onDisconnect.removeListener(disconnectListener) | |
| } | |
| }, |
| async onEnd(aborted) { | ||
| try { | ||
| if (!aborted) { | ||
| port.postMessage({ done: true }) | ||
| } | ||
| } finally { | ||
| port.onMessage.removeListener(messageListener) | ||
| port.onDisconnect.removeListener(disconnectListener) | ||
| } | ||
| }, |
There was a problem hiding this comment.
To maintain consistency with openai-compatible-core.mjs, we should also persist the streamed partial answer into the session history when the request is aborted. We should wrap the port.postMessage call in a try...catch block in case the port is disconnected.
| async onEnd(aborted) { | |
| try { | |
| if (!aborted) { | |
| port.postMessage({ done: true }) | |
| } | |
| } finally { | |
| port.onMessage.removeListener(messageListener) | |
| port.onDisconnect.removeListener(disconnectListener) | |
| } | |
| }, | |
| async onEnd(aborted) { | |
| try { | |
| if (!aborted) { | |
| port.postMessage({ done: true }) | |
| } else if (answer) { | |
| pushRecord(session, question, answer) | |
| try { | |
| port.postMessage({ session }) | |
| } catch (e) { | |
| console.warn('[waylaidwanderer-api] Failed to post session on abort:', e) | |
| } | |
| } | |
| } finally { | |
| port.onMessage.removeListener(messageListener) | |
| port.onDisconnect.removeListener(disconnectListener) | |
| } | |
| }, |
PR Summary by QodoUnstick Stop UI on abort/proxy disconnect during streaming
AI Description
Diagram
High-Level Assessment
Files changed (9)
|
There was a problem hiding this comment.
Pull request overview
This PR addresses a UX issue where the conversation UI can remain stuck in a “Stop”/generating state when a streaming request is interrupted (slow request, abort, or proxy tab disconnect). It does so by treating aborts as a controlled end-of-stream across SSE handling, API clients, and the background proxy bridge so the UI reliably unlocks.
Changes:
- Update
fetchSSEto treatAbortErrorasonEnd(true)and to swallowonEnderrors so disconnect-related cleanup doesn’t surface as failures. - Update multiple API clients to avoid posting
{ done: true }on abort, and in some cases persist partial answers / reject stream promises on abort. - Add a per-port
_generatingflag in the background proxy bridge to post{ done: true }if the proxy disconnects mid-generation, unlocking the UI.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/utils/fetch-sse.mjs | Treat abort as a normal end (onEnd(true)) and swallow onEnd errors to avoid false failures on disconnect. |
| src/services/clients/claude/index.mjs | Reject the stream promise on abort so cancellation is handled as cancellation. |
| src/services/apis/waylaidwanderer-api.mjs | Avoid posting {done:true} on abort; ensure listeners are removed in finally. |
| src/services/apis/openai-compatible-core.mjs | On abort, persist streamed partial answer into session history without re-sending done. |
| src/services/apis/moonshot-web.mjs | Reject the stream promise on abort so cancellation is handled as cancellation. |
| src/services/apis/claude-api.mjs | Avoid posting {done:true} on abort; ensure listeners are removed in finally. |
| src/services/apis/chatgpt-web.mjs | Avoid posting {done:true} on abort; ensure controller cleanup runs in finally. |
| src/services/apis/azure-openai-api.mjs | Avoid posting {done:true} on abort; ensure listeners are removed in finally. |
| src/background/index.mjs | Track in-flight generation via _generating and post {done:true} on proxy disconnect to unlock UI. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/utils/fetch-sse.mjs (1)
3-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSolid abort-aware refactor; minor duplication to consider.
The
isAbortError→onEnd(true)/ elseonError(err)pattern is repeated verbatim in the fetch try/catch (lines 12-25) and the reader-loop try/catch (lines 60-71). Extracting a small helper would reduce duplication and ensure future edits to abort handling stay in sync.♻️ Optional extraction
+async function handleTermination(err, onEnd, onError) { + if (isAbortError(err)) { + try { + await onEnd(true) + } catch (e) { + console.warn('[fetch-sse] onEnd threw during abort:', e) + } + return + } + await onError(err) +}Then replace both catch blocks with
return await handleTermination(err, onEnd, onError).Also applies to: 38-76
🤖 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 `@src/utils/fetch-sse.mjs` around lines 3 - 25, The abort-handling logic is duplicated in fetchSSE’s fetch and reader-loop catch paths, so extract it into a shared helper to keep behavior in sync. Add a small termination helper near isAbortError that takes the error plus onEnd/onError, handles AbortError by calling onEnd(true) with the same warning guard, and otherwise forwards to onError(err). Then replace both catch blocks in fetchSSE and the reader loop with that helper so the abort-aware flow is centralized.
🤖 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.
Nitpick comments:
In `@src/utils/fetch-sse.mjs`:
- Around line 3-25: The abort-handling logic is duplicated in fetchSSE’s fetch
and reader-loop catch paths, so extract it into a shared helper to keep behavior
in sync. Add a small termination helper near isAbortError that takes the error
plus onEnd/onError, handles AbortError by calling onEnd(true) with the same
warning guard, and otherwise forwards to onError(err). Then replace both catch
blocks in fetchSSE and the reader loop with that helper so the abort-aware flow
is centralized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 03267618-0a1d-49a2-acf9-34e73abcc2e1
📒 Files selected for processing (9)
src/background/index.mjssrc/services/apis/azure-openai-api.mjssrc/services/apis/chatgpt-web.mjssrc/services/apis/claude-api.mjssrc/services/apis/moonshot-web.mjssrc/services/apis/openai-compatible-core.mjssrc/services/apis/waylaidwanderer-api.mjssrc/services/clients/claude/index.mjssrc/utils/fetch-sse.mjs
Code Review by Qodo
1. Stale disconnect requestId
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| function isAbortError(err) { | ||
| if (!err || typeof err !== 'object') return false | ||
| const name = typeof err.name === 'string' ? err.name : '' | ||
| return name === 'AbortError' | ||
| } |
There was a problem hiding this comment.
2. Abort detection inconsistent 🐞 Bug ☼ Reliability
fetchSSE()’s new isAbortError() only checks err.name === 'AbortError', which is inconsistent with other repo abort handling that also matches 'aborted' messages; this increases the risk that some cancellations are treated as errors (onError) instead of the new abort end-path (onEnd(true)).
Agent Prompt
### Issue description
`src/utils/fetch-sse.mjs` introduces `isAbortError()` that only checks `err.name === 'AbortError'`. Elsewhere in the codebase, abort detection is broader (name OR message contains “aborted/aborterror”), indicating aborts are not always represented with the canonical DOMException name.
### Issue Context
If aborts are misclassified, `fetchSSE()` will route them to `onError()` rather than `onEnd(true)`, undermining the PR’s new “abort behaves like end” semantics.
### Fix Focus Areas
- src/utils/fetch-sse.mjs[3-7]
### Suggested fix
Align `fetch-sse` abort detection with the repo’s existing pattern:
- Also inspect `err.message` (lowercased) for substrings like `aborted` / `aborterror`.
- Keep the `AbortError` name check.
Optionally factor this into a shared util (to avoid divergent abort detection across files).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
a78600b to
24b663f
Compare
There was a problem hiding this comment.
Code Review
This pull request improves error handling and state management during stream aborts and port disconnections across various API services and the background script. It introduces tracking for active generation states, gracefully handles disconnected ports, and updates fetchSSE to trigger abort cleanups. The review feedback suggests using optional chaining when checking incoming port messages to prevent potential TypeError crashes, and recommends making the isAbortError check in fetch-sse.mjs more robust to match the implementation in wrappers.mjs.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| function isAbortError(err) { | ||
| if (!err || typeof err !== 'object') return false | ||
| const name = typeof err.name === 'string' ? err.name : '' | ||
| return name === 'AbortError' | ||
| } |
There was a problem hiding this comment.
This implementation of isAbortError is less robust than the one defined in src/services/wrappers.mjs. In some environments or under certain network conditions, abort errors might be wrapped or thrown as generic errors with messages containing 'aborted' rather than having the exact name 'AbortError'. If not recognized as an abort, it will trigger onError and show a disruptive error message to the user. Consider aligning this with the more robust version.
| function isAbortError(err) { | |
| if (!err || typeof err !== 'object') return false | |
| const name = typeof err.name === 'string' ? err.name : '' | |
| return name === 'AbortError' | |
| } | |
| function isAbortError(err) { | |
| if (!err || typeof err !== 'object') return false | |
| const name = typeof err.name === 'string' ? err.name : '' | |
| const message = typeof err.message === 'string' ? err.message.toLowerCase() : '' | |
| return name === 'AbortError' || message.includes('aborted') || message.includes('aborterror') | |
| } |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/services/wrappers.mjs (1)
86-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the catch-block parameter to avoid shadowing
postError.The catch parameter
postErrorshadows the outerpostErrorarrow function. While not a bug today (the catch block only logs), a future retry attempt inside the catch would silently fail sincepostErrorwould resolve to the caught error object, not the function.♻️ Proposed refactor
const postError = (error) => { try { port.postMessage({ error }) - } catch (postError) { - console.warn('[handlePortError] Failed to post error:', postError) + } catch (postErr) { + console.warn('[handlePortError] Failed to post error:', postErr) } }🤖 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 `@src/services/wrappers.mjs` around lines 86 - 92, The catch parameter inside postError currently shadows the outer postError function, which can cause confusion and future retry attempts to reference the wrong value. Rename the catch-block parameter in the postError helper to a distinct name, and keep the logic in the port.postMessage error handling unchanged so the outer postError symbol remains callable.
🤖 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.
Nitpick comments:
In `@src/services/wrappers.mjs`:
- Around line 86-92: The catch parameter inside postError currently shadows the
outer postError function, which can cause confusion and future retry attempts to
reference the wrong value. Rename the catch-block parameter in the postError
helper to a distinct name, and keep the logic in the port.postMessage error
handling unchanged so the outer postError symbol remains callable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 744fd4af-3c65-45c7-90d6-acd8dad9a719
📒 Files selected for processing (13)
src/background/index.mjssrc/services/apis/azure-openai-api.mjssrc/services/apis/chatgpt-web.mjssrc/services/apis/claude-api.mjssrc/services/apis/moonshot-web.mjssrc/services/apis/openai-compatible-core.mjssrc/services/apis/waylaidwanderer-api.mjssrc/services/clients/claude/index.mjssrc/services/wrappers.mjssrc/utils/fetch-sse.mjstests/unit/services/apis/openai-api-compat.test.mjstests/unit/services/handle-port-error.test.mjstests/unit/utils/fetch-sse.test.mjs
🚧 Files skipped from review as they are similar to previous changes (8)
- src/services/apis/chatgpt-web.mjs
- src/services/apis/azure-openai-api.mjs
- src/services/apis/claude-api.mjs
- src/services/apis/openai-compatible-core.mjs
- src/background/index.mjs
- src/services/apis/moonshot-web.mjs
- src/services/apis/waylaidwanderer-api.mjs
- src/services/clients/claude/index.mjs
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24b663f398
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Code review by qodo was updated up to the latest commit 24b663f |
24b663f to
2689a9a
Compare
There was a problem hiding this comment.
Code Review
This pull request improves abort and error handling across various API clients and SSE fetching, ensuring proper cleanup of listeners and preventing crashes when ports are disconnected. Key changes include tracking port generation state, handling aborted states in onEnd callbacks, and safely catching disconnected port errors. The review feedback highlights a potential TypeError in the background script if a message is null or undefined, and suggests optimizing a React useEffect hook by using a useRef to prevent unnecessary listener churn during stream updates.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| } | ||
| } | ||
| }, [conversationItemData]) | ||
| }, [port, conversationItemData]) |
There was a problem hiding this comment.
Including conversationItemData in the dependency array of this useEffect causes the message listener to be removed and re-registered on every single stream chunk (since conversationItemData updates continuously during generation).\n\nTo optimize performance and avoid unnecessary listener churn, you can store the latest conversationItemData in a useRef (e.g., conversationItemDataRef.current = conversationItemData on every render) and reference conversationItemDataRef.current inside portMessageListener. This allows you to safely remove conversationItemData from the useEffect dependency array so the listener is only registered once per port.
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING| File | Line | Issue | Files Reviewed (changed in this increment)
Fix these issues in Kilo Cloud Previous Review Summaries (6 snapshots, latest commit 9a038f6)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 9a038f6)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING| File | Line | Issue | Files Reviewed (20 files)
Fix these issues in Kilo Cloud Previous review (commit f56ab05)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING| File | Line | Issue | Files Reviewed (20 files)
Fix these issues in Kilo Cloud Previous review (commit a4e1939)Status: No New Issues Found (incremental pass) | Recommendation: Address existing active reviewer comments before merge OverviewThis is an incremental review of commits
Incremental Analysis
Files Reviewed (14 files)
Previous review (commit 1aed904)Status: No Issues Found | Recommendation: Merge Files Reviewed (14 files)
Previous review (commit 12ea56a)Status: No Issues Found | Recommendation: Merge Files Reviewed (14 files)
Previous review (commit 2689a9a)Status: No New Issues Found | Recommendation: Merge Files Reviewed (14 files)
Reviewed by hy3:free · Input: 107.5K · Output: 9.9K · Cached: 774K |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/components/ConversationCard/index.jsx (1)
496-499: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding
portto theonUpdateeffect dependency array.The
onUpdateeffect at lines 119–121 callsprops.onUpdate(port, session, conversationItemData)but only depends on[session, conversationItemData]. This PR introduces a new code path (lines 496–499) whereportchanges viasetPort(Browser.runtime.connect()). In the current clear-conversation flow the parent still receives the new port becausesetConversationItemData([])(line 506) andsetSession(newSession)(line 513) also fire, triggering the effect. However, this is fragile — if a future change removes either of those state updates, the parent would retain a stale, disconnected port reference incurrentPort.current.Adding
portto the dependency array is a one-line fix that makes the parent-child port contract robust regardless of what else changes in the flow.♻️ Proposed fix for `onUpdate` effect deps
useEffect(() => { if (props.onUpdate) props.onUpdate(port, session, conversationItemData) - }, [session, conversationItemData]) + }, [port, session, conversationItemData])🤖 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 `@src/components/ConversationCard/index.jsx` around lines 496 - 499, The onUpdate effect in ConversationCard is using a stale port because it only depends on session and conversationItemData even though the clear-conversation flow can replace port via setPort(Browser.runtime.connect()). Update the dependency array for the effect that calls props.onUpdate(port, session, conversationItemData) to include port so the parent always receives the current connection reference. Use the onUpdate effect in ConversationCard and the port state update path around Browser.runtime.connect() as the key symbols to locate the fix.
🤖 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.
Nitpick comments:
In `@src/components/ConversationCard/index.jsx`:
- Around line 496-499: The onUpdate effect in ConversationCard is using a stale
port because it only depends on session and conversationItemData even though the
clear-conversation flow can replace port via setPort(Browser.runtime.connect()).
Update the dependency array for the effect that calls props.onUpdate(port,
session, conversationItemData) to include port so the parent always receives the
current connection reference. Use the onUpdate effect in ConversationCard and
the port state update path around Browser.runtime.connect() as the key symbols
to locate the fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 24c7a4bf-32b3-4a1d-8a36-9a663af8d596
📒 Files selected for processing (14)
src/background/index.mjssrc/components/ConversationCard/index.jsxsrc/services/apis/azure-openai-api.mjssrc/services/apis/chatgpt-web.mjssrc/services/apis/claude-api.mjssrc/services/apis/moonshot-web.mjssrc/services/apis/openai-compatible-core.mjssrc/services/apis/waylaidwanderer-api.mjssrc/services/clients/claude/index.mjssrc/services/wrappers.mjssrc/utils/fetch-sse.mjstests/unit/services/apis/openai-api-compat.test.mjstests/unit/services/handle-port-error.test.mjstests/unit/utils/fetch-sse.test.mjs
✅ Files skipped from review due to trivial changes (1)
- src/services/apis/moonshot-web.mjs
🚧 Files skipped from review as they are similar to previous changes (12)
- tests/unit/services/handle-port-error.test.mjs
- tests/unit/utils/fetch-sse.test.mjs
- src/services/apis/chatgpt-web.mjs
- src/services/apis/openai-compatible-core.mjs
- src/services/apis/claude-api.mjs
- src/services/apis/azure-openai-api.mjs
- src/services/apis/waylaidwanderer-api.mjs
- src/services/clients/claude/index.mjs
- src/background/index.mjs
- tests/unit/services/apis/openai-api-compat.test.mjs
- src/services/wrappers.mjs
- src/utils/fetch-sse.mjs
|
Code review by qodo was updated up to the latest commit 2689a9a |
2689a9a to
12ea56a
Compare
There was a problem hiding this comment.
Code Review
This pull request improves error handling, connection cleanup, and abort behavior across background scripts, API clients, and SSE fetching utilities. It introduces state tracking for active generation, safely handles disconnected ports, and ensures proper cleanup of listeners when streams are aborted. The review feedback highlights a performance issue in ConversationCard where useEffect re-runs on every streamed token, and suggests defensive programming improvements such as optional chaining for msg and a default fallback for onEnd in fetchSSE.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| } | ||
| } | ||
| }, [conversationItemData]) | ||
| }, [port, conversationItemData]) |
There was a problem hiding this comment.
Including conversationItemData in the dependency array of this useEffect causes the message listener to be removed and re-added on every single streamed token, which is highly inefficient and can cause performance issues or race conditions.\n\nTo optimize this, you can remove conversationItemData from the dependency array and use a useRef to keep track of the latest conversationItemData without triggering effect re-runs.\n\nFor example, add a ref at the top of the component:\njavascript\nconst conversationItemDataRef = useRef(conversationItemData);\nconversationItemDataRef.current = conversationItemData;\n\nAnd then update portMessageListener to use conversationItemDataRef.current instead of conversationItemData.
| }, [port, conversationItemData]) | |
| }, [port]) |
| } | ||
|
|
||
| export async function fetchSSE(resource, options) { | ||
| const { onMessage, onStart, onEnd, onError, ...fetchOptions } = options |
There was a problem hiding this comment.
Provide a default empty function for onEnd in the destructuring assignment to prevent runtime TypeError crashes if onEnd is omitted from the options object.
| const { onMessage, onStart, onEnd, onError, ...fetchOptions } = options | |
| const { onMessage, onStart, onEnd = () => {}, onError, ...fetchOptions } = options |
|
Code review by qodo was updated up to the latest commit 12ea56a |
12ea56a to
1aed904
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/utils/fetch-sse.test.mjs (1)
115-175: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting shared mock setup to reduce duplication.
All three new tests repeat the same
t.mock.method(console, 'debug', ...)andt.mock.method(globalThis, 'fetch', ...)boilerplate. A small helper orbeforeEach/t.beforehook could reduce this to a single setup call per test.♻️ Optional refactor
+function setupSseMock(t, chunk = 'data: {"delta":"A"}\n\n') { + t.mock.method(console, 'debug', () => {}) + t.mock.method(globalThis, 'fetch', async () => createMockSseResponse([chunk])) +} + test('fetchSSE propagates onEnd errors on normal completion', async (t) => { - t.mock.method(console, 'debug', () => {}) - t.mock.method(globalThis, 'fetch', async () => createMockSseResponse(['data: {"delta":"A"}\n\n'])) + setupSseMock(t)test('fetchSSE propagates onStart errors', async (t) => { - t.mock.method(console, 'debug', () => {}) - t.mock.method(globalThis, 'fetch', async () => createMockSseResponse(['data: {"delta":"A"}\n\n'])) + setupSseMock(t)test('fetchSSE propagates onMessage errors', async (t) => { - t.mock.method(console, 'debug', () => {}) - t.mock.method(globalThis, 'fetch', async () => createMockSseResponse(['data: {"delta":"A"}\n\n'])) + setupSseMock(t)🤖 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/unit/utils/fetch-sse.test.mjs` around lines 115 - 175, The new fetchSSE tests duplicate the same console.debug and globalThis.fetch mock setup across multiple cases; extract that boilerplate into a shared helper or a test hook so each test only defines its specific onStart/onMessage/onEnd/onError behavior. Update the fetchSSE test block to reuse the common setup around createMockSseResponse and t.mock.method, keeping the individual assertions focused on the error propagation scenarios.
🤖 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.
Nitpick comments:
In `@tests/unit/utils/fetch-sse.test.mjs`:
- Around line 115-175: The new fetchSSE tests duplicate the same console.debug
and globalThis.fetch mock setup across multiple cases; extract that boilerplate
into a shared helper or a test hook so each test only defines its specific
onStart/onMessage/onEnd/onError behavior. Update the fetchSSE test block to
reuse the common setup around createMockSseResponse and t.mock.method, keeping
the individual assertions focused on the error propagation scenarios.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4fa04106-dc1a-4a63-9b09-5f5cef0393f1
📒 Files selected for processing (14)
src/background/index.mjssrc/components/ConversationCard/index.jsxsrc/services/apis/azure-openai-api.mjssrc/services/apis/chatgpt-web.mjssrc/services/apis/claude-api.mjssrc/services/apis/moonshot-web.mjssrc/services/apis/openai-compatible-core.mjssrc/services/apis/waylaidwanderer-api.mjssrc/services/clients/claude/index.mjssrc/services/wrappers.mjssrc/utils/fetch-sse.mjstests/unit/services/apis/openai-api-compat.test.mjstests/unit/services/handle-port-error.test.mjstests/unit/utils/fetch-sse.test.mjs
🚧 Files skipped from review as they are similar to previous changes (13)
- src/services/apis/waylaidwanderer-api.mjs
- tests/unit/services/handle-port-error.test.mjs
- src/services/apis/azure-openai-api.mjs
- src/services/clients/claude/index.mjs
- src/services/apis/chatgpt-web.mjs
- src/services/wrappers.mjs
- src/utils/fetch-sse.mjs
- src/services/apis/claude-api.mjs
- src/services/apis/openai-compatible-core.mjs
- src/components/ConversationCard/index.jsx
- src/services/apis/moonshot-web.mjs
- src/background/index.mjs
- tests/unit/services/apis/openai-api-compat.test.mjs
There was a problem hiding this comment.
Code Review
This pull request introduces robust error handling, abort state propagation, and connection management across various API services, background scripts, and SSE utilities. It ensures that aborted streams are handled gracefully, partial responses are preserved when appropriate, and disconnected port errors are ignored or caught safely. The reviewer feedback focuses on optimizing the React components in ConversationCard. Specifically, it suggests avoiding potential memory leaks by setting a flag directly on the port object instead of using replacedPortRef, and highlights a performance issue where the message listener is re-registered on every stream chunk update due to conversationItemData being in the dependency array.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if (replacedPortRef.current === port) { | ||
| replacedPortRef.current = null | ||
| return | ||
| } |
There was a problem hiding this comment.
Instead of using a useRef (replacedPortRef) to keep track of the manually disconnected port (which can lead to a memory leak if the old port object is retained in the ref indefinitely when onDisconnect does not fire locally), you can simply attach a temporary flag directly to the port object itself (e.g., port._replaced = true).
This avoids the need for replacedPortRef entirely and prevents any potential memory leaks.
if (port._replaced) return| if (!useForegroundFetch) { | ||
| replacedPortRef.current = port | ||
| port.disconnect() | ||
| setPort(Browser.runtime.connect()) | ||
| } |
There was a problem hiding this comment.
To accompany the simplified port replacement logic and avoid memory leaks from holding onto the old port in a ref, you can set a flag directly on the port object before disconnecting.
| if (!useForegroundFetch) { | |
| replacedPortRef.current = port | |
| port.disconnect() | |
| setPort(Browser.runtime.connect()) | |
| } | |
| if (!useForegroundFetch) { | |
| port._replaced = true | |
| port.disconnect() | |
| setPort(Browser.runtime.connect()) | |
| } |
| } | ||
| } | ||
| }, [conversationItemData]) | ||
| }, [port, conversationItemData]) |
There was a problem hiding this comment.
Including conversationItemData in the dependency array of this useEffect causes the message listener to be removed and re-registered on port.onMessage on every single stream chunk update (since conversationItemData changes with every new token/chunk). This is highly inefficient and can cause performance degradation or race conditions during rapid streaming.
To fix this, you can use a useRef to hold the latest portMessageListener (or the latest conversationItemData) and remove conversationItemData from the dependency array so that the listener is only registered once per port connection.
For example, you can define a ref for the listener:
const listenerRef = useRef(portMessageListener)
listenerRef.current = portMessageListenerAnd then in the useEffect:
useEffect(() => {
if (useForegroundFetch) {
return () => {}
} else {
const listener = (msg) => listenerRef.current(msg)
port.onMessage.addListener(listener)
return () => {
port.onMessage.removeListener(listener)
}
}
}, [port])| }, [port, conversationItemData]) | |
| }, [port]) |
| function isAbortError(err) { | ||
| if (!err || typeof err !== 'object') return false | ||
| const name = typeof err.name === 'string' ? err.name : '' | ||
| return name === 'AbortError' | ||
| } |
|
Code review by qodo was updated up to the latest commit 1aed904 |
a4e1939 to
f56ab05
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces robust state management for proxy generation and session handling, adding mechanisms to track active generations, suppress reconnect errors on interruption, and handle superseded session requests across various web APIs. It also updates SSE fetching to handle abort signals gracefully and adds comprehensive unit tests. The review feedback focuses on defensive programming, recommending safety checks to ensure message objects are defined before accessing their properties and suggesting a fallback default for session.conversationRecords to prevent potential runtime errors.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| export function markProxyGenerationFinishedFromMessage(port, message) { | ||
| if (message.stoppedGenerationId !== undefined || (!message.error && !message.done)) return false | ||
| markProxyGenerationFinished(port) | ||
| return true | ||
| } |
There was a problem hiding this comment.
To prevent potential runtime TypeError exceptions, add a safety check to ensure message is defined before accessing its properties. This is a good defensive programming practice.
| export function markProxyGenerationFinishedFromMessage(port, message) { | |
| if (message.stoppedGenerationId !== undefined || (!message.error && !message.done)) return false | |
| markProxyGenerationFinished(port) | |
| return true | |
| } | |
| export function markProxyGenerationFinishedFromMessage(port, message) { | |
| if (!message || message.stoppedGenerationId !== undefined || (!message.error && !message.done)) return false | |
| markProxyGenerationFinished(port) | |
| return true | |
| } |
| export function isSupersededGenerationMessage(message, latestSupersededGenerationId) { | ||
| return ( | ||
| message.stoppedGenerationId !== undefined && | ||
| message.stoppedGenerationId <= latestSupersededGenerationId | ||
| ) | ||
| } |
There was a problem hiding this comment.
Use optional chaining message?.stoppedGenerationId to safely handle cases where message might be null or undefined, preventing potential runtime crashes.
| export function isSupersededGenerationMessage(message, latestSupersededGenerationId) { | |
| return ( | |
| message.stoppedGenerationId !== undefined && | |
| message.stoppedGenerationId <= latestSupersededGenerationId | |
| ) | |
| } | |
| export function isSupersededGenerationMessage(message, latestSupersededGenerationId) { | |
| return ( | |
| message?.stoppedGenerationId !== undefined && | |
| message.stoppedGenerationId <= latestSupersededGenerationId | |
| ) | |
| } |
| export function finalizeInterruptedSession(session, answer, retryRecord = null) { | ||
| if (!answer) { | ||
| if (!session.isRetry && !retryRecord) return session | ||
| const lastRecord = session.conversationRecords.at(-1) | ||
| const shouldRestoreRetryRecord = | ||
| retryRecord && | ||
| (lastRecord?.question !== retryRecord.question || lastRecord?.answer !== retryRecord.answer) | ||
| return { | ||
| ...session, | ||
| conversationRecords: shouldRestoreRetryRecord | ||
| ? [...session.conversationRecords, { ...retryRecord }] | ||
| : session.conversationRecords, | ||
| isRetry: false, | ||
| } | ||
| } | ||
| const updatedSession = { | ||
| ...session, | ||
| conversationRecords: session.conversationRecords.map((record) => ({ ...record })), | ||
| } | ||
| pushRecord(updatedSession, session.question, answer) | ||
| updatedSession.isRetry = false | ||
| return updatedSession | ||
| } |
There was a problem hiding this comment.
To ensure robustness and prevent potential runtime errors, default session.conversationRecords to an empty array if it is undefined or null. This is a recommended defensive programming practice.
export function finalizeInterruptedSession(session, answer, retryRecord = null) {
const conversationRecords = session.conversationRecords || []
if (!answer) {
if (!session.isRetry && !retryRecord) return session
const lastRecord = conversationRecords.at(-1)
const shouldRestoreRetryRecord =
retryRecord &&
(lastRecord?.question !== retryRecord.question || lastRecord?.answer !== retryRecord.answer)
return {
...session,
conversationRecords: shouldRestoreRetryRecord
? [...conversationRecords, { ...retryRecord }]
: conversationRecords,
isRetry: false,
}
}
const updatedSession = {
...session,
conversationRecords: conversationRecords.map((record) => ({ ...record })),
}
pushRecord(updatedSession, session.question, answer)
updatedSession.isRetry = false
return updatedSession
}| const onMessage = async (msg) => { | ||
| console.debug('received msg', msg) | ||
| if (msg.stop) { |
There was a problem hiding this comment.
Add a safety check to ensure msg is defined before checking msg.stop to prevent potential runtime TypeError exceptions if a null or undefined message is received on the port.
| const onMessage = async (msg) => { | |
| console.debug('received msg', msg) | |
| if (msg.stop) { | |
| const onMessage = async (msg) => { | |
| console.debug('received msg', msg) | |
| if (!msg) return | |
| if (msg.stop) { |
| async onEnd(aborted) { | ||
| try { | ||
| if (!aborted) { | ||
| port.postMessage({ done: true }) | ||
| } | ||
| } finally { | ||
| cleanController() | ||
| } | ||
| }, | ||
| async onError(resp) { | ||
| cleanController() | ||
| if (resp instanceof Error) throw resp | ||
| if (resp.status === 403) { | ||
| throw new Error('CLOUDFLARE') | ||
| } | ||
| const error = await resp.json().catch(() => ({})) | ||
| throw new Error( | ||
| !isEmpty(error) ? JSON.stringify(error) : `${resp.status} ${resp.statusText}`, | ||
| ) | ||
| }, |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
|
|
||
| export function finalizeInterruptedSession(session, answer, retryRecord = null) { | ||
| if (!answer) { | ||
| if (!session.isRetry && !retryRecord) return session |
There was a problem hiding this comment.
WARNING: Interrupted retry with no partial answer appends a blank record
When a done (proxy disconnect or stop) arrives while session.isRetry is true and no partial answer/retry record is present (partialAnswerRef and retryRecordRef are null/empty — the retry case where createRetrySession set isRetry: retryRecord === null), the early return session guard is skipped because it requires !session.isRetry, and the code falls through to pushRecord(updatedSession, session.question, ''), appending a blank answer record to conversation history.
Fix by also bailing out when there is no actual answer to persist, e.g. change the guard to if (!answer && !retryRecord) { ... } or add && !answer handling so an interrupted retry without any streamed content does not record an empty answer.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
Code review by qodo was updated up to the latest commit f56ab05 |
f56ab05 to
9a038f6
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces robust request and generation tracking across the extension's background scripts, content scripts, and UI components. By tagging messages with unique proxy and request generation IDs, the system can now reliably identify and discard superseded or stale requests, prevent duplicate or out-of-order responses, and gracefully handle aborted streams. It also adds robust error handling for disconnected ports, enhances the SSE client to support abort signals, and introduces comprehensive unit tests covering these new synchronization states. I have no feedback to provide as there are no review comments.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| port.onMessage.removeListener(portMessageListener) | ||
| } | ||
| } | ||
| }, [conversationItemData]) | ||
| }, [port, conversationItemData]) |
| postError(t('Please login at https://bing.com first')) | ||
| else postError(message) | ||
| } else { | ||
| const errMsg = JSON.stringify(err) ?? 'unknown error' |
| function postProxySession(port, session, requestGenerationId) { | ||
| const proxyGenerationId = (port._proxyGenerationId ?? 0) + 1 | ||
| port.proxy.postMessage({ | ||
| session, | ||
| proxyGenerationId, | ||
| ...(requestGenerationId === undefined ? {} : { requestGenerationId }), | ||
| }) | ||
| markProxyGenerationStarted(port, proxyGenerationId, requestGenerationId) |
There was a problem hiding this comment.
1. Stale disconnect requestid 🐞 Bug ≡ Correctness
postProxySession() posts to port.proxy before recording the new request generation state, but
the proxy disconnect handler tags its {done:true, proxyDisconnected:true} using the stored
_proxyRequestGenerationId. If port.proxy.postMessage() throws or a disconnect happens before
markProxyGenerationStarted() runs, the disconnect completion can be tagged with a previous
requestGenerationId and be dropped by the UI’s superseded-request filter, leaving the current
request stuck.
Agent Prompt
### Issue description
`postProxySession()` currently calls `port.proxy.postMessage(...)` before persisting the new request’s generation bookkeeping via `markProxyGenerationStarted(...)`. Disconnect/error paths (e.g. proxy disconnect completion) rely on `port._proxyRequestGenerationId` when tagging messages; if the proxy post fails or disconnect occurs before the bookkeeping is committed, the emitted completion can be tagged with a stale `requestGenerationId` and dropped by the UI.
### Issue Context
- Disconnect completion is tagged using `tagProxyRequestGeneration(port, ...)`, which reads `port._proxyRequestGenerationId`.
- The UI drops messages whose `requestGenerationId` doesn’t match the current request.
### Fix Focus Areas
- src/background/index.mjs[64-72]
- src/background/index.mjs[184-200]
- src/background/proxy-generation-state.mjs[1-12]
### Suggested fix
Make the request/proxy generation state update atomic with respect to disconnect/error tagging:
1) Move the state update ahead of `port.proxy.postMessage(...)` (or at least set `_proxyRequestGenerationId` and `_proxyGenerationId` before posting).
2) If you need `_generating` to reflect only *successful* dispatch, consider:
- pre-setting IDs used for tagging (request/proxy generation ids),
- then setting `_generating=true` only after a successful post,
- and reverting/clearing IDs if the post fails.
3) Add/adjust unit tests to cover the failure path where `postMessage` throws and a subsequent disconnect/max-retry path emits a tagged message (ensuring it uses the current requestGenerationId).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 9a038f6 |
- background: unlock the conversation UI (post {done:true}) when the
ChatGPT/Claude/etc. proxy tab disconnects mid-generation; use a
per-port _generating flag so only in-flight requests are affected.
- fetch-sse: treat abort as onEnd(true) and swallow onEnd errors so a
closed port on disconnect is not reported as a failure.
- openai-compatible-core: on abort, persist the streamed partial answer
into session history (pushRecord + post {session}) without re-sending
the done signal.
- moonshot-web/claude client: on abort, reject the stream promise so
cancellation is treated as cancellation, not a successful completion.
9a038f6 to
4421eee
Compare
| await handleCallbackError(err) | ||
| } | ||
| } | ||
| await onEnd() |
There was a problem hiding this comment.
WARNING: Normal-completion onEnd() is not guarded, contradicting the PR's stated goal
Unlike the abort-path onEnd(true) calls (lines 48-52), the final await onEnd() here is unwrapped. If an onEnd implementation throws after the stream ends — e.g. finish() calls port.postMessage({ answer: null, done: true, session }) (openai-compatible-core.mjs:121) or { done: true } (claude-api.mjs:84, chatgpt-web.mjs:422) on a port that was just disconnected — the rejection propagates out of fetchSSE(), reaching registerPortListener's executor catch → handlePortError, which can surface a spurious error to the UI or leave it in a stuck state. This is exactly the disconnect-after-done failure the PR claims to fix.
Wrap it to match the abort path, e.g.:
| await onEnd() | |
| try { | |
| await onEnd() | |
| } catch (e) { | |
| console.warn('[fetch-sse] onEnd threw during completion:', e) | |
| } |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Summary by CodeRabbit
{ done: true }is sent only on normal completion, with consistent abort-aware behavior and guaranteed cleanup.