Fix UI stuck on Stop when request is slow or proxy disconnects#995
Fix UI stuck on Stop when request is slow or proxy disconnects#995PeterDaveHello wants to merge 1 commit into
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.
| if (aborted) { | ||
| if (answer) { | ||
| pushRecord(session, question, answer) | ||
| port.postMessage({ session }) | ||
| } |
There was a problem hiding this comment.
When the stream is aborted due to a port disconnection (e.g., when the popup or sidebar is closed), calling port.postMessage will throw an error because the port is already closed. We should wrap this call in a try...catch block to prevent unhandled exceptions.
if (aborted) {
if (answer) {
pushRecord(session, question, answer)
try {
port.postMessage({ session })
} catch (e) {
console.warn('[openai-compatible-core] Failed to post session on abort:', e)
}
}
}| 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) | |
| } | |
| }, |
| async onEnd(aborted) { | ||
| try { | ||
| if (!aborted) { | ||
| port.postMessage({ done: true }) | ||
| } | ||
| } finally { | ||
| cleanController() | ||
| } | ||
| }, |
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('[chatgpt-web] Failed to post session on abort:', e)
}
}
} finally {
cleanController()
}
},| console.debug('[background] Main port closed; skipping proxy message.') | ||
| return | ||
| } | ||
| if (msg.done || msg.error) port._generating = false |
There was a problem hiding this comment.
Use optional chaining msg?.done and msg?.error to prevent potential TypeError if msg is null or undefined, adhering to defensive programming guidelines.
| if (msg.done || msg.error) port._generating = false | |
| if (msg?.done || msg?.error) port._generating = false |
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.
| async onEnd(aborted = false) { | ||
| if (!finished) { | ||
| finish() | ||
| if (aborted) { | ||
| if (answer) { | ||
| pushRecord(session, question, answer) |
![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. Done lacks session update
|
![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
PeterDaveHello
force-pushed
the
fix/stop-abort-ui-unstick
branch
from
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.
| console.debug('[background] Main port closed; skipping proxy message.') | ||
| return | ||
| } | ||
| if (msg.done || msg.error) port._generating = false |
There was a problem hiding this comment.
If msg is null or undefined, accessing msg.done or msg.error will throw a TypeError and crash the port's message listener. Since msg can potentially be null or non-object in extension messaging, it is safer to use optional chaining here.
| if (msg.done || msg.error) port._generating = false | |
| if (msg?.done || msg?.error) port._generating = false |
| 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".
| const shouldPostSession = Boolean(answer) || session.isRetry | ||
| if (shouldPostSession) { |
There was a problem hiding this comment.
When an OpenAI-compatible stream is aborted after producing any partial text, this branch always posts the old session back to the UI. The same stop message is used by controls that immediately replace the session, such as Clear Conversation in ConversationCard; because postMessage({ stop: true }) resolves before the background abort finishes, the late { session } here can restore the just-cleared/in-flight conversation and make the next prompt include stale history even though the UI was reset.
Useful? React with 👍 / 👎.
|
Code review by qodo was updated up to the latest commit 24b663f |
PeterDaveHello
force-pushed
the
fix/stop-abort-ui-unstick
branch
from
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.
| console.debug('[background] Main port closed; skipping proxy message.') | ||
| return | ||
| } | ||
| if (msg.done || msg.error) port._generating = false |
There was a problem hiding this comment.
If msg is null or undefined, accessing msg.done or msg.error will throw a TypeError and crash the background script's message listener. Since msg can potentially be null/undefined in extension message passing, you should use optional chaining here to prevent runtime crashes.
| if (msg.done || msg.error) port._generating = false | |
| if (msg?.done || msg?.error) port._generating = false |
| } | ||
| } | ||
| }, [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.
| await postMessage({ stop: true }) | ||
| if (!useForegroundFetch) { | ||
| port.disconnect() | ||
| setPort(Browser.runtime.connect()) | ||
| } |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (14 files)
Previous Review Summaries (2 snapshots, latest commit 12ea56a)Current summary above is authoritative. Previous snapshots are kept for context only. 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 step-3.7-flash · Input: 343.8K · Output: 40K · Cached: 1.2M |
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 |
PeterDaveHello
force-pushed
the
fix/stop-abort-ui-unstick
branch
from
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]) |
| console.debug('[background] Main port closed; skipping proxy message.') | ||
| return | ||
| } | ||
| if (msg.done || msg.error) port._generating = false |
There was a problem hiding this comment.
Use optional chaining (msg?.done and msg?.error) to prevent potential TypeError if msg is null or undefined, ensuring robust defensive programming.
| if (msg.done || msg.error) port._generating = false | |
| if (msg?.done || msg?.error) port._generating = false |
| } | ||
|
|
||
| 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 |
| try { | ||
| while (!(result = await reader.read()).done) { | ||
| const chunk = result.value | ||
| if (!hasStarted) { | ||
| const str = new TextDecoder().decode(chunk) |
| if (port._generating) { | ||
| port._generating = false | ||
| if (!port._isClosed) { | ||
| try { | ||
| port.postMessage({ done: true }) | ||
| } catch (e) { | ||
| console.warn('[background] Error posting done on proxy disconnect:', e) | ||
| } | ||
| } |
There was a problem hiding this comment.
1. Done lacks session update 🐞 Bug ≡ Correctness
When the proxy tab disconnects mid-generation, setPortProxy() posts { done: true } without a `{
session }`, so the UI marks the answer complete and becomes ready while
session.conversationRecords remains stale. As a result, saving/exporting or persisting the
conversation can omit the partial answer the user saw.
Agent Prompt
### Issue description
On proxy disconnect during an in-flight generation, the background sends `{ done: true }` to the UI to unstick it, but does not provide an updated `{ session }`. The UI only updates `session` when `msg.session` is present, so the partial assistant answer that was streamed to the UI is not persisted into `session.conversationRecords`, which is later used for “Save Conversation” and for rehydration/persistence.
### Issue Context
- Background disconnect path sends done-only.
- UI marks done/ready on `msg.done`, but does not update `session` unless `msg.session` exists.
- Save/export and initial rehydration are based on `session.conversationRecords`.
### Fix Focus Areas
- src/background/index.mjs[154-169]
- src/components/ConversationCard/index.jsx[97-114]
- src/components/ConversationCard/index.jsx[175-186]
- src/components/ConversationCard/index.jsx[562-574]
- src/pages/IndependentPanel/App.jsx[169-179]
### Suggested fix
Implement a way to persist the currently displayed partial answer into the session when a proxy disconnect triggers a done-only terminal signal.
Two viable approaches:
1) **UI-side (recommended):** In `ConversationCard.portMessageListener`, when receiving `msg.done === true` **and** `msg.session` is missing, derive the last Q/A from current UI state (or track latest answer text in a ref) and `pushRecord`-equivalent into a cloned `session`, then `setSession(updatedSession)` before setting ready.
- Guard against double-persist if a subsequent `{session}` arrives.
2) **Protocol change:** Add a marker flag from background, e.g. `{ done: true, proxyDisconnected: true }`, so the UI can distinguish this path from other done-only signals (e.g., stop) and apply special persistence logic only for proxy disconnects.
Either approach should ensure `session.conversationRecords` reflects the partial answer that remains visible in the UI, so Save/rehydration/persistence does not drop it.
ⓘ 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 12ea56a |
- 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.
PeterDaveHello
force-pushed
the
fix/stop-abort-ui-unstick
branch
from
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 |
Summary by CodeRabbit
{ done: true }is only sent on normal completion, with consistent abort behavior and guaranteed cleanup.