fix web socket error.#12
Conversation
…connection-issues fix: keep websocket sessions alive
WalkthroughAdds server-side WebSocket liveness and in-band chat ping/pong, implements client-side heartbeat with pong timeout and exponential reconnect backoff, refactors Gemini CLI spawn/spec to promise-based flows with pre-spawn session abort and simplified streaming output, and applies minor presentational/accessibility tweaks to timestamp elements. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Browser as Client
participant WS as WebSocket
participant Server as App Server
Note over Browser,WS: Connection established
Browser->>WS: Open
WS-->>Browser: Open ack
Note over Browser: Start keep-alive (every 30s)
loop Every 30s
Browser->>Server: {"type":"ping"} %% in-band ping message
Note over Browser: Start 10s pong timeout
alt Server responds
Server-->>Browser: {"type":"pong"}
Browser->>Browser: Clear pong timeout
else No response (timeout)
Browser->>WS: Close socket
Browser->>Browser: Schedule reconnect (exp backoff)
end
end
Note over Server: Native ws heartbeat
Server->>Server: ws.isAlive = true on connection
Server->>Server: ws.on('pong', heartbeat)
sequenceDiagram
autonumber
participant HTTP as Server Route
participant CLI as spawnGemini()
participant Sess as Session Manager
participant Proc as Child Process
participant WS as WebSocket
HTTP->>CLI: spawnGemini(command, options, ws)
CLI->>Sess: abortGeminiSession(prevSessionId)
Sess-->>CLI: aborted / none
CLI->>Proc: spawn child process (model/debug args)
Proc-->>CLI: stdout chunks
CLI->>CLI: filter lines -> filteredOutput
CLI-->>WS: send {type:"gemini-response", data: filteredOutput}
Proc-->>CLI: exit
CLI-->>HTTP: resolve Promise
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
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.
Summary of Changes
Hello @qqq-tech, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request implements a robust heartbeat and reconnection mechanism for WebSocket connections. The changes introduce a client-side ping-pong system to actively monitor connection liveness and a server-side response to these pings. Furthermore, the client's reconnection strategy is enhanced with an exponential backoff, significantly improving the stability and resilience of real-time communication by preventing stale connections and managing network fluctuations more effectively.
Highlights
- WebSocket Liveness Monitoring: A client-side heartbeat mechanism is introduced, sending periodic 'ping' messages and expecting 'pong' responses from the server to confirm connection activity.
- Server-Side Heartbeat Response: The server now actively responds to client 'ping' messages with 'pong', completing the heartbeat loop.
- Resilient Reconnection Strategy: The client's reconnection attempts now utilize an exponential backoff algorithm, gradually increasing delay between retries to prevent network congestion and improve stability.
- Connection Timeout for Stale Links: The client will automatically close a WebSocket connection if a 'pong' response is not received within a set timeout after sending a 'ping', ensuring inactive connections are properly terminated.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a robust heartbeat and reconnection mechanism for WebSockets, which is a great improvement for connection stability. The changes include an application-level ping/pong, exponential backoff for retries on the client, and a fix for the server-side heartbeat logic. The implementation is solid, but I've identified a potential memory leak in the new client-side reconnection logic that could lead to attempts to update state on an unmounted component. My review comment provides a detailed explanation and a suggested fix for this issue.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/websocket.js (1)
15-28: Fix unmount cleanup:wscaptured stale; use a ref to reliably close the active socketThe cleanup in the empty-deps effect uses a stale
wsfrom initial render, risking an unclosed socket on unmount/navigation. Track the live socket in a ref and close that instead.Apply:
export function useWebSocket() { const [ws, setWs] = useState(null); const [messages, setMessages] = useState([]); const [isConnected, setIsConnected] = useState(false); const reconnectTimeoutRef = useRef(null); + const wsRef = useRef(null); const reconnectAttemptsRef = useRef(0); const heartbeatIntervalRef = useRef(null); const pongTimeoutRef = useRef(null); useEffect(() => { connect(); return () => { if (reconnectTimeoutRef.current) { clearTimeout(reconnectTimeoutRef.current); } if (heartbeatIntervalRef.current) { clearInterval(heartbeatIntervalRef.current); } if (pongTimeoutRef.current) { clearTimeout(pongTimeoutRef.current); } - if (ws) { - ws.close(); - } + if (wsRef.current) { + try { wsRef.current.close(); } catch {} + } }; }, []); @@ websocket.onopen = () => { setIsConnected(true); setWs(websocket); + wsRef.current = websocket; reconnectAttemptsRef.current = 0; @@ websocket.onclose = () => { setIsConnected(false); setWs(null); + wsRef.current = null;Also applies to: 71-84, 101-118
🧹 Nitpick comments (3)
src/utils/websocket.js (3)
76-84: Avoid overlapping pong timeoutsClear any existing
pongTimeoutRefbefore scheduling a new one to prevent stray timers if the interval timing ever changes.heartbeatIntervalRef.current = setInterval(() => { if (websocket.readyState === WebSocket.OPEN) { websocket.send(JSON.stringify({ type: 'ping' })); + if (pongTimeoutRef.current) { + clearTimeout(pongTimeoutRef.current); + } pongTimeoutRef.current = setTimeout(() => { try { websocket.close(); } catch { /* ignore */ } }, 10000); } }, 30000);
67-70: Token in query string — consider alternativesQuery params can end up in logs. If feasible for your deployment, consider cookie-based auth (httpOnly, secure, sameSite) or using
Sec-WebSocket-Protocolto pass a bearer token. Keep current flow for browsers if constraints apply.
59-65: Silent catches reduce diagnosabilityWrap these with a dev-only logger (e.g., gated by
import.meta.env.DEV) to aid troubleshooting without noisy production logs.Example:
-} catch { - // console.warn('Could not fetch server config, falling back to current host with API server port'); +} catch { + if (import.meta?.env?.DEV) console.warn('[ws] Falling back to local ws URL');Also applies to: 96-98, 120-126
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
server/index.js(2 hunks)src/utils/websocket.js(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (3)
server/index.js (2)
425-426: Server-side liveness wiring — LGTMInitializing
ws.isAliveand bindingpongtoheartbeatcorrectly integrates with the existing interval ping.
451-454: In-band ping/pong handling — LGTMEarly return avoids unnecessary processing; this complements control-frame heartbeat well.
src/utils/websocket.js (1)
86-99: Heartbeat, pong short-circuit, and exponential backoff — LGTM
- Skipping
pongmessages keeps the UI log clean.- Backoff calculation and cap are sensible; attempts reset on open.
Also applies to: 112-118
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
server/gemini-cli.js (1)
265-273: Good fix: preserve streamed spacing; consider buffering to avoid split-debug leakageJoining without trim and removing the extra newline insertion is correct for chunk-accurate rendering. However, filtering per raw chunk risks leaking debug lines when markers (e.g., “[DEBUG]”) span chunk boundaries. Buffer and filter only complete lines.
Example (outside the shown hunk) using the existing outputBuffer:
// replace the per-chunk split with line-buffering outputBuffer += rawOutput; const parts = outputBuffer.split('\n'); const complete = parts.slice(0, -1); // full lines outputBuffer = parts[parts.length - 1] || ''; // leftover partial line const filteredLines = complete.filter(line => { if (line.includes('[DEBUG]') || line.includes('Flushing log events') || line.includes('Clearcut response') || line.includes('[MemoryDiscovery]') || line.includes('[BfsFileSearch]')) { return false; } return true; }); const filteredOutput = filteredLines.join('\n'); if (filteredOutput) { fullResponse += filteredOutput; ws.send(JSON.stringify({ type: 'gemini-response', data: { type: 'message', content: filteredOutput }})); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
server/gemini-cli.js(1 hunks)src/components/ChatInterface.jsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
server/gemini-cli.js (1)
server/projects.js (1)
filteredLines(453-460)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/ChatInterface.jsx (1)
1248-1261: Bug: toolResult shape mismatch breaks Tool Result rendering for loaded sessionsconvertSessionMessages sets toolResult to a string + separate flags, but the renderer expects an object with {content,isError,timestamp}. Also missing toolId. Tool results from history won’t display.
Apply:
- } else if (part.type === 'tool_use') { + } else if (part.type === 'tool_use') { // Get the corresponding tool result const toolResult = toolResults.get(part.id); converted.push({ type: 'assistant', content: '', timestamp: msg.timestamp || new Date().toISOString(), isToolUse: true, toolName: part.name, - toolInput: JSON.stringify(part.input), - toolResult: toolResult ? (typeof toolResult.content === 'string' ? toolResult.content : JSON.stringify(toolResult.content)) : null, - toolError: toolResult?.isError || false, - toolResultTimestamp: toolResult?.timestamp || new Date() + toolInput: JSON.stringify(part.input), + toolId: part.id, + toolResult: toolResult + ? { + content: typeof toolResult.content === 'string' + ? toolResult.content + : JSON.stringify(toolResult.content), + isError: !!toolResult.isError, + timestamp: toolResult.timestamp || new Date() + } + : null });
♻️ Duplicate comments (2)
src/components/ChatInterface.jsx (2)
1536-1551: Preserve whitespace and avoid in-place mutation when streaming parts (array branch)Current code drops whitespace-only chunks and mutates state in place; this can break spacing and React.memo re-renders. Use immutable update and keep whitespace.
Apply:
- } else if (part.type === 'text' && part.text?.trim()) { + } else if (part.type === 'text' && typeof part.text === 'string' && part.text.length > 0) { // Append streaming text to the last assistant message if possible setChatMessages(prev => { const updated = [...prev]; const last = updated[updated.length - 1]; if (last && last.type === 'assistant' && !last.isToolUse) { - last.content += part.text; + updated[updated.length - 1] = { + ...last, + content: (last.content || '') + part.text + }; } else { updated.push({ type: 'assistant', content: part.text, timestamp: new Date() }); } return updated; });
1554-1569: Same issue for string-content branch: keep whitespace + immutable updateMirror the fix above for consistency.
Apply:
- } else if (typeof messageData.content === 'string' && messageData.content.trim()) { + } else if (typeof messageData.content === 'string' && messageData.content.length > 0) { // Append streaming text to the last assistant message if possible setChatMessages(prev => { const updated = [...prev]; const last = updated[updated.length - 1]; if (last && last.type === 'assistant' && !last.isToolUse) { - last.content += messageData.content; + updated[updated.length - 1] = { + ...last, + content: (last.content || '') + messageData.content + }; } else { updated.push({ type: 'assistant', content: messageData.content, timestamp: new Date() }); } return updated; });
🧹 Nitpick comments (4)
src/components/ChatInterface.jsx (4)
1310-1315: Avoid redundant state churn on scroll eventsMinor: guard against no-op sets to reduce scroll-handler overhead.
Apply:
- const handleScroll = useCallback(() => { + const handleScroll = useCallback(() => { if (scrollContainerRef.current) { const nearBottom = isNearBottom(); - setIsAutoScrollPaused(!nearBottom); + setIsAutoScrollPaused(prev => (prev !== !nearBottom ? !nearBottom : prev)); } }, [isNearBottom]);
2251-2253: Typo: missing “u” in underline classButton won’t be underlined.
Apply:
- className="ml-1 text-gemini-600 hover:text-gemini-800 nderline" + className="ml-1 text-gemini-600 hover:text-gemini-800 underline"
1392-1398: Persisting full chat (incl. images) to localStorage each change may hit quota and jank UIConsider throttling writes and excluding large blobs (e.g., image data) from persisted state.
I can propose a lightweight serializer and a write debounce if you want.
1647-1661: Clear pendingSessionId on abort to avoid stale protection stateIf a session is aborted before completion, pendingSessionId remains set.
Apply:
case 'session-aborted': setIsLoading(false); setCanAbortSession(false); setGeminiStatus(null); + sessionStorage.removeItem('pendingSessionId');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/components/ChatInterface.jsx(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/ChatInterface.jsx (1)
src/App.jsx (1)
autoScrollToBottom(64-67)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (5)
src/components/ChatInterface.jsx (5)
1091-1094: Good addition: explicit auto-scroll pause stateClear naming and inline docs make behavior obvious.
1295-1295: Resetting pause on programmatic scroll is correctEnsures resume-to-bottom works predictably after user sends or clicks button.
1808-1824: Auto-scroll pause logic is applied correctlyOnly scrolls when auto-scroll is enabled and user hasn’t scrolled up. Looks good.
1827-1834: Initial-load scroll behaviorBoolean dependency to trigger once when messages first appear is appropriate; reset of pause state is correct.
2005-2006: Resetting pause on send ensures the response stays in viewGood UX touch.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/components/ChatInterface.jsx (1)
1536-1550: Fix streaming updates: avoid mutating state and don’t trim away whitespace-only chunksDirectly mutating
last.contentrisks missed re-renders with React.memo;.trim()drops intentional spacing from streamed chunks. Use immutable update and check length instead of trim.Apply:
- } else if (part.type === 'text' && part.text?.trim()) { + } else if (part.type === 'text' && typeof part.text === 'string' && part.text.length > 0) { // Append streaming text to the last assistant message if possible setChatMessages(prev => { const updated = [...prev]; const last = updated[updated.length - 1]; if (last && last.type === 'assistant' && !last.isToolUse) { - last.content += part.text; + updated[updated.length - 1] = { + ...last, + content: (last.content || '') + part.text + }; } else { updated.push({ type: 'assistant', content: part.text, timestamp: new Date() }); } return updated; }); } - } else if (typeof messageData.content === 'string' && messageData.content.trim()) { + } else if (typeof messageData.content === 'string' && messageData.content.length > 0) { // Append streaming text to the last assistant message if possible setChatMessages(prev => { const updated = [...prev]; const last = updated[updated.length - 1]; if (last && last.type === 'assistant' && !last.isToolUse) { - last.content += messageData.content; + updated[updated.length - 1] = { + ...last, + content: (last.content || '') + messageData.content + }; } else { updated.push({ type: 'assistant', content: messageData.content, timestamp: new Date() }); } return updated; }); }Run to confirm no other mutable appends / trim-gating remain:
#!/bin/bash rg -n -C2 -e '\.content\s*\+\=' -e '\.trim\(\)' --type=js --type=jsxAlso applies to: 1554-1568
🧹 Nitpick comments (2)
src/components/ChatInterface.jsx (2)
1091-1095: Auto-scroll pause/resume looks good; use layout effect for pre-paint captureNice UX. To avoid one-frame scroll jank when preserving position, capture pre-paint via
useLayoutEffect.- // Capture scroll position before render when auto-scroll is disabled or paused - useEffect(() => { + // Capture scroll position before paint when auto-scroll is disabled or paused + useLayoutEffect(() => { if ((isAutoScrollPaused || !autoScrollToBottom) && scrollContainerRef.current) { const container = scrollContainerRef.current; scrollPositionRef.current = { height: container.scrollHeight, top: container.scrollTop }; } - }); + });Additionally (outside this hunk):
- Import
useLayoutEffect:// at line 19 import import React, { useState, useEffect, useLayoutEffect, useRef, useMemo, useCallback, memo } from 'react';Optional: add a passive scroll listener to reduce main-thread work:
// where you add/remove the 'scroll' listener scrollContainer.addEventListener('scroll', handleScroll, { passive: true });Also applies to: 1295-1295, 1299-1307, 1310-1316, 1792-1802, 1804-1819, 1826-1827, 1999-2001, 2326-2336
2246-2246: Typo in className: “nderline” → “underline”This prevents the underline style.
- className="ml-1 text-gemini-600 hover:text-gemini-800 nderline" + className="ml-1 text-gemini-600 hover:text-gemini-800 underline"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/components/ChatInterface.jsx(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/ChatInterface.jsx (1)
src/App.jsx (1)
autoScrollToBottom(64-67)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/ChatInterface.jsx (1)
1251-1261: Fix: toolResult shape mismatch (string vs object) breaks renderer
convertSessionMessagesassignstoolResultas a string, but the renderer expects an object withcontentandisError. Loaded sessions won’t show tool results correctly.converted.push({ type: 'assistant', content: '', timestamp: msg.timestamp || new Date().toISOString(), isToolUse: true, toolName: part.name, toolInput: JSON.stringify(part.input), - toolResult: toolResult ? (typeof toolResult.content === 'string' ? toolResult.content : JSON.stringify(toolResult.content)) : null, - toolError: toolResult?.isError || false, - toolResultTimestamp: toolResult?.timestamp || new Date() + toolResult: toolResult + ? { + content: typeof toolResult.content === 'string' + ? toolResult.content + : JSON.stringify(toolResult.content), + isError: !!toolResult.isError, + timestamp: toolResult.timestamp + } + : null, + // (Optional) keep legacy fields until all render paths are unified + toolError: toolResult?.isError || false, + toolResultTimestamp: toolResult?.timestamp || new Date() });
♻️ Duplicate comments (2)
src/components/ChatInterface.jsx (2)
1535-1549: Fix: Immutable update for streaming append + don’t drop whitespace-only chunksMutating
last.contentin place can be skipped byReact.memo, preventing updates. Also,trim()drops spacing-only chunks, breaking streamed formatting.Apply:
- } else if (part.type === 'text' && part.text?.trim()) { + } else if (part.type === 'text' && typeof part.text === 'string' && part.text.length > 0) { // Append streaming text to the last assistant message if possible setChatMessages(prev => { const updated = [...prev]; const last = updated[updated.length - 1]; if (last && last.type === 'assistant' && !last.isToolUse) { - last.content += part.text; + updated[updated.length - 1] = { + ...last, + content: (last.content || '') + part.text + }; } else { updated.push({ type: 'assistant', content: part.text, timestamp: new Date() }); } return updated; });
1553-1567: Fix: Same issue for plain string streaming pathMirror the immutable update and whitespace preservation here.
- } else if (typeof messageData.content === 'string' && messageData.content.trim()) { + } else if (typeof messageData.content === 'string' && messageData.content.length > 0) { // Append streaming text to the last assistant message if possible setChatMessages(prev => { const updated = [...prev]; const last = updated[updated.length - 1]; if (last && last.type === 'assistant' && !last.isToolUse) { - last.content += messageData.content; + updated[updated.length - 1] = { + ...last, + content: (last.content || '') + messageData.content + }; } else { updated.push({ type: 'assistant', content: messageData.content, timestamp: new Date() }); } return updated; });
🧹 Nitpick comments (2)
src/components/ChatInterface.jsx (2)
1298-1306: Nit: Make bottom detection tolerant to fractional pixelsA tolerance of 1 can mis-classify “at bottom” on HiDPI/elastic scroll. Use a slightly larger epsilon and ceil scrollTop.
- // Treat as bottom only when the user is effectively at the end - return Math.abs(scrollHeight - scrollTop - clientHeight) <= 1; + // Treat as bottom with small tolerance; account for fractional scrollTop + const delta = scrollHeight - Math.ceil(scrollTop) - clientHeight; + return delta <= 2;
1309-1315: Nit: Avoid no-op state sets during scrollPrevent redundant state updates on hot scroll paths.
- const handleScroll = useCallback(() => { + const handleScroll = useCallback(() => { if (scrollContainerRef.current) { const atBottom = isAtBottom(); - setIsAutoScrollPaused(!atBottom); + setIsAutoScrollPaused(prev => (prev === !atBottom ? prev : !atBottom)); } }, [isAtBottom]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/components/ChatInterface.jsx(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/ChatInterface.jsx (1)
src/App.jsx (1)
autoScrollToBottom(64-67)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (6)
src/components/ChatInterface.jsx (6)
1091-1094: LGTM: Clear “pause auto-scroll” stateGood rename and semantics; matches the UI/UX intent.
1292-1295: LGTM: Clicking “scroll to bottom” unpauses auto-scrollResetting
isAutoScrollPausedhere is correct.
1792-1795: LGTM: Auto-scroll only when not pausedGuards respect user scroll state during streaming.
1802-1803: LGTM: Reset pause on initial loadPrevents “stuck paused” on first render.
1976-1977: LGTM: Unpause auto-scroll on user sendEnsures the response is visible immediately.
2302-2313: LGTM: Visible “scroll to bottom” affordance when pausedNice UX touch; placement beside mode indicator works.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/components/ChatInterface.jsx (1)
1536-1551: Immutable update + don’t drop whitespace-only streamed chunksDirectly mutating last.content can prevent React.memo re-renders; trim() loses meaningful spacing in streams. Use an immutable replace and keep whitespace-only chunks.
Apply:
- } else if (part.type === 'text' && part.text?.trim()) { + } else if (part.type === 'text' && typeof part.text === 'string' && part.text.length > 0) { // Append streaming text to the last assistant message if possible setChatMessages(prev => { const updated = [...prev]; const last = updated[updated.length - 1]; - if (last && last.type === 'assistant' && !last.isToolUse) { - last.content += part.text; + if (last && last.type === 'assistant' && !last.isToolUse) { + updated[updated.length - 1] = { + ...last, + content: (last.content || '') + part.text + }; } else { updated.push({ type: 'assistant', content: part.text, timestamp: new Date() }); } return updated; }); }- } else if (typeof messageData.content === 'string' && messageData.content.trim()) { + } else if (typeof messageData.content === 'string' && messageData.content.length > 0) { // Append streaming text to the last assistant message if possible setChatMessages(prev => { const updated = [...prev]; const last = updated[updated.length - 1]; - if (last && last.type === 'assistant' && !last.isToolUse) { - last.content += messageData.content; + if (last && last.type === 'assistant' && !last.isToolUse) { + updated[updated.length - 1] = { + ...last, + content: (last.content || '') + messageData.content + }; } else { updated.push({ type: 'assistant', content: messageData.content, timestamp: new Date() }); } return updated; }); }Also applies to: 1555-1569
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/components/ChatInterface.jsx(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/ChatInterface.jsx (1)
src/App.jsx (1)
autoScrollToBottom(64-67)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (1)
src/components/ChatInterface.jsx (1)
1091-1094: Auto-scroll pause state: LGTMClear naming and intent; good addition.
| scrollContainerRef.current.scrollTop = scrollContainerRef.current.scrollHeight; | ||
| } | ||
| setIsUserScrolledUp(false); | ||
| setIsAutoScrollPaused(false); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Auto-scroll never resumes when user manually returns to bottom
handleScroll only pauses; it doesn’t unpause when the user scrolls back to bottom. This leaves isAutoScrollPaused stuck true until programmatic scrollToBottom runs. Add an else branch to clear the pause when at bottom. Button UX relies on this.
Apply:
const handleScroll = useCallback(() => {
- if (!scrollContainerRef.current) {
- return;
- }
- if (!isAtBottom()) {
- setIsAutoScrollPaused(true);
- }
+ if (!scrollContainerRef.current) return;
+ const atBottom = isAtBottom();
+ if (!atBottom) {
+ setIsAutoScrollPaused(true);
+ } else {
+ setIsAutoScrollPaused(false);
+ }
}, [isAtBottom]);Also applies to: 1298-1306, 1309-1316, 1794-1796, 1804-1805, 2304-2314
🤖 Prompt for AI Agents
In src/components/ChatInterface.jsx around line 1294 (and also apply same fix to
ranges 1298-1306, 1309-1316, 1794-1796, 1804-1805, 2304-2314), the scroll
handler only sets isAutoScrollPaused(true) when user scrolls away from bottom
but never clears the paused flag when the user scrolls back; add an else branch
to detect when the scroll position is at or near the bottom and call
setIsAutoScrollPaused(false) (use the same bottom-threshold logic already
present), ensuring the auto-scroll state is unpaused when the user manually
returns to the bottom; replicate this change in the other listed
handlers/locations for consistent behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/components/ChatInterface.jsx (3)
1309-1318: Auto-scroll pause now resumes when back at bottom.This addresses the prior “never resumes” issue.
1538-1553: Do not mutate last message; also don’t drop whitespace-only chunks.In-place mutation can be skipped by React.memo, and trim() collapses streamed spacing. Use immutable update and a length check.
Apply:
- } else if (part.type === 'text' && part.text?.trim()) { + } else if (part.type === 'text' && typeof part.text === 'string' && part.text.length > 0) { // Append streaming text to the last assistant message if possible setChatMessages(prev => { const updated = [...prev]; const last = updated[updated.length - 1]; if (last && last.type === 'assistant' && !last.isToolUse) { - last.content += part.text; + updated[updated.length - 1] = { + ...last, + content: (last.content || '') + part.text + }; } else { updated.push({ type: 'assistant', content: part.text, timestamp: new Date() }); } return updated; });
1556-1571: Same issue in the non-array content path.Mirror the immutable update and preserve whitespace.
Apply:
- } else if (typeof messageData.content === 'string' && messageData.content.trim()) { + } else if (typeof messageData.content === 'string' && messageData.content.length > 0) { // Append streaming text to the last assistant message if possible setChatMessages(prev => { const updated = [...prev]; const last = updated[updated.length - 1]; if (last && last.type === 'assistant' && !last.isToolUse) { - last.content += messageData.content; + updated[updated.length - 1] = { + ...last, + content: (last.content || '') + messageData.content + }; } else { updated.push({ type: 'assistant', content: messageData.content, timestamp: new Date() }); } return updated; });
🧹 Nitpick comments (1)
src/components/ChatInterface.jsx (1)
2306-2316: Accessibility: add aria-label to the scroll button.Small a11y win; no visual change.
Apply:
- {isAutoScrollPaused && chatMessages.length > 0 && ( + {isAutoScrollPaused && chatMessages.length > 0 && ( <button onClick={scrollToBottom} className="w-8 h-8 bg-gemini-600 hover:bg-gemini-800 text-white rounded-full shadow-lg flex items-center justify-center transition-all duration-200 hover:scale-105 focus:outline-none focus:ring-2 focus:ring-gemini-500 focus:ring-offset-2 dark:ring-offset-gray-800" - title="Scroll to bottom" + title="Scroll to bottom" + aria-label="Scroll to bottom" >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/components/ChatInterface.jsx(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (6)
src/components/ChatInterface.jsx (6)
1091-1094: Good state rename and purpose.Clearer semantics than isUserScrolledUp; fits the new flow.
1281-1296: Resetting pause on programmatic scroll is correct.Clears manual pause when we intentionally snap to bottom.
1298-1306: Bottom detection looks solid.The <= 5px tolerance is pragmatic for sub-pixel jitter.
1795-1799: Auto-scroll gated by pause flag is correct.Prevents jumping while user is reading older messages.
1806-1807: Reset pause on initial load.Matches expected UX on first render.
1979-1981: Clearing pause on send is right.Ensures the response streams into view.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/components/ChatInterface.jsx (3)
1309-1318: Auto-scroll pause toggling now resumes at bottom — niceThis addresses the earlier “never resumes” feedback.
1560-1575: Same issue for aggregated string content pathMirror the immutable update and whitespace preservation here too.
- } else if (typeof messageData.content === 'string' && messageData.content.trim()) { + } else if (typeof messageData.content === 'string' && messageData.content.length > 0) { // Append streaming text to the last assistant message if possible setChatMessages(prev => { const updated = [...prev]; const last = updated[updated.length - 1]; if (last && last.type === 'assistant' && !last.isToolUse) { - last.content += messageData.content; + updated[updated.length - 1] = { + ...last, + content: (last.content || '') + messageData.content + }; } else { updated.push({ type: 'assistant', content: messageData.content, timestamp: new Date() }); } return updated; });
1542-1556: Don’t mutate last message in-place; also don’t drop whitespace-only chunksMutating
lastbreaks memoized child re-renders;.trim()drops spacing the server now preserves. Use immutable update and accept empty/whitespace chunks.Apply:
- } else if (part.type === 'text' && part.text?.trim()) { + } else if (part.type === 'text' && typeof part.text === 'string' && part.text.length > 0) { // Append streaming text to the last assistant message if possible setChatMessages(prev => { const updated = [...prev]; const last = updated[updated.length - 1]; if (last && last.type === 'assistant' && !last.isToolUse) { - last.content += part.text; + updated[updated.length - 1] = { + ...last, + content: (last.content || '') + part.text + }; } else { updated.push({ type: 'assistant', content: part.text, timestamp: new Date() }); } return updated; });
🧹 Nitpick comments (3)
server/gemini-cli.js (1)
243-248: Streaming join over full buffer is O(n²) and unbounded in memory; switch to incremental filtering with leftover handlingRejoining the entire accumulated buffer on every chunk scales poorly and grows memory without bound for long streams. Keep a small "leftover" line and process only the newly received chunk; append only the filtered delta. This also avoids repeatedly scanning historical output.
Example approach (outside selected range for illustration):
// define outside handler let leftover = ''; let filteredSoFarLen = 0; geminiProcess.stdout.on('data', (buf) => { hasReceivedOutput = true; clearTimeout(timeout); const chunk = leftover + buf.toString(); const parts = chunk.split('\n'); leftover = parts.pop(); // keep incomplete tail (no trailing newline) const filtered = parts.filter(line => !( line.includes('[DEBUG]') || line.includes('Flushing log events') || line.includes('Clearcut response') || line.includes('[MemoryDiscovery]') || line.includes('[BfsFileSearch]') )).join('\n') + '\n'; // re-add newline for completed lines if (filtered.length) { fullResponse += filtered; safeSend(ws, { type: 'gemini-response', data: { type: 'message', content: filtered } }); } }); // on 'close', optionally flush leftover if needed (after filtering)If you prefer to keep current approach, at minimum cap
outputBuffer(e.g., slice last N KB) or periodically discard processed prefixes to prevent runaway memory.Also applies to: 251-267
src/components/ChatInterface.jsx (2)
1298-1306: Bottom detection: consider centralizing thresholdLogic is fine; optionally extract the 5px tolerance into a constant to keep it consistent across handlers/tests.
- return Math.abs(scrollHeight - scrollTop - clientHeight) <= 5; + const BOTTOM_TOLERANCE_PX = 5; + return Math.abs(scrollHeight - scrollTop - clientHeight) <= BOTTOM_TOLERANCE_PX;
2309-2320: Add accessible label to the “scroll to bottom” buttonScreen readers need a label; tooltip alone isn’t sufficient.
- {isAutoScrollPaused && chatMessages.length > 0 && ( + {isAutoScrollPaused && chatMessages.length > 0 && ( <button onClick={scrollToBottom} className="w-8 h-8 bg-gemini-600 hover:bg-gemini-800 text-white rounded-full shadow-lg flex items-center justify-center transition-all duration-200 hover:scale-105 focus:outline-none focus:ring-2 focus:ring-gemini-500 focus:ring-offset-2 dark:ring-offset-gray-800" - title="Scroll to bottom" + title="Scroll to bottom" + aria-label="Scroll to bottom" >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
server/gemini-cli.js(2 hunks)src/components/ChatInterface.jsx(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
server/gemini-cli.js (4)
server/routes/git.js (1)
lines(136-136)server/projects.js (3)
lines(441-441)filteredLines(453-460)sessionManager(211-211)server/routes/mcp.js (2)
lines(221-221)lines(266-266)server/sessionManager.js (1)
sessionManager(181-181)
src/components/ChatInterface.jsx (1)
src/App.jsx (1)
autoScrollToBottom(64-67)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (6)
server/gemini-cli.js (1)
291-304: Session creation handshake: good sequencingCreating the session after first output and updating
activeGeminiProcesseswith the real ID avoids orphaned process entries. Looks good.src/components/ChatInterface.jsx (5)
1091-1094: Rename and semantics: good replacement for isUserScrolledUp → isAutoScrollPausedState naming now reflects behavior. No issues.
1456-1458: Pre-capture bottom state before mutating messages — goodCapturing
shouldScrollbefore processing avoids race conditions.
1711-1715: Auto-scroll after render: timing looks goodThe small delay avoids jank and respects the pre-captured bottom state.
1809-1811: Initial load scroll: explicit unpause is correctResetting
isAutoScrollPausedon first load avoids sticky paused state.
1982-1984: Reset scroll pause on user send — good UXEnsures streamed response will auto-follow unless the user scrolls away.
| ws.send(JSON.stringify({ | ||
| type: 'gemini-response', | ||
| data: { | ||
| type: 'message', | ||
| content: filteredOutput | ||
| content: newContent | ||
| } | ||
| })); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard ws.send to prevent crashes when the client disconnects mid-stream
Multiple sends happen after long-running work (timeout, stderr, complete, error). If the socket is closed, ws.send throws and can take down the request. Wrap sends in a small helper that checks readyState and swallows benign errors.
Apply locally here:
- ws.send(JSON.stringify({
+ safeSend(ws, {
type: 'gemini-response',
data: {
type: 'message',
content: newContent
}
- }));
+ });Add this helper once (outside the selected range):
function safeSend(ws, payload) {
try {
if (ws && ws.readyState === 1 /* OPEN */) {
ws.send(JSON.stringify(payload));
}
} catch (_) {
// no-op: client likely disconnected
}
}Use safeSend for all emits in this file: timeout (lines 228-233), stderr (327-331), complete (347-351), and error (382-385).
🤖 Prompt for AI Agents
In server/gemini-cli.js around lines 277-283, ws.send calls can throw if the
client disconnects mid-stream; add a safeSend helper function once near the top
of the file (outside the posted ranges) that checks ws.readyState === 1 (OPEN),
JSON.stringify the payload and wraps ws.send in a try/catch that swallows
errors, then replace all direct ws.send(...) calls in this file with
safeSend(ws, payload) — specifically update the emits at timeout (lines
~228-233), the current block (277-283), stderr (327-331), complete (347-351),
and error (382-385) to use safeSend so closed sockets won't crash the request.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
server/gemini-cli.js (2)
219-221: Avoid double-closing stdin; keep it open when interactive; close only when using --prompt.stdin.end() is called unconditionally at Line 220 and again conditionally at Lines 395–396. Remove the unconditional call to prevent double-end and to preserve interactive mode.
- // Close stdin to signal we're done sending input - geminiProcess.stdin.end(); + // Do not close stdin here; interactive mode may need it. We close it below only if using --prompt.Also applies to: 391-399
376-389: Clear timeout on process error to prevent leaking the timer.Timeout is cleared on stdout and on close but not on 'error'.
geminiProcess.on('error', (error) => { // console.error('Gemini CLI process error:', error); // Clean up process reference on error const finalSessionId = capturedSessionId || sessionId || processKey; activeGeminiProcesses.delete(finalSessionId); + clearTimeout(timeout); - ws.send(JSON.stringify({ + safeSend(ws, { type: 'gemini-error', error: error.message - })); + }); reject(error); });src/components/ChatInterface.jsx (1)
2491-2500: Prevent duplicate sends: onMouseDown/onTouchStart + form onSubmit causes double submit.Clicking the submit button triggers handleSubmit via mouse/touch and again via form submit. Remove the mouse/touch handlers and rely on onSubmit (and Enter key handling) to send once.
<button type="submit" disabled={!input.trim() || isLoading} - onMouseDown={(e) => { - e.preventDefault(); - handleSubmit(e); - }} - onTouchStart={(e) => { - e.preventDefault(); - handleSubmit(e); - }} className="absolute right-2 top-1/3 transform -translate-y-1/2 w-12 h-12 sm:w-12 sm:h-12 bg-gemini-500 hover:bg-gemini-600 disabled:bg-gray-400 disabled:cursor-not-allowed rounded-full flex items-center justify-center transition-colors focus:outline-none focus:ring-2 focus:ring-gemini-500 focus:ring-offset-2 dark:ring-offset-gray-800" >
♻️ Duplicate comments (2)
server/gemini-cli.js (1)
228-233: Guard ws.send to avoid crashes on closed sockets (re-raising prior feedback).Direct ws.send calls can throw if the client disconnected mid-stream. Add a safeSend wrapper and use it for all emits listed in these ranges.
Add once near the top (outside these ranges):
function safeSend(ws, payload) { try { if (ws && ws.readyState === 1 /* OPEN */) { ws.send(JSON.stringify(payload)); } } catch { // no-op } }Apply this diff within the selected ranges:
@@ - ws.send(JSON.stringify({ + safeSend(ws, { type: 'gemini-error', error: 'Gemini CLI timeout - no response received' - })); + }); @@ - ws.send(JSON.stringify({ + safeSend(ws, { type: 'gemini-response', data: { type: 'message', content: newContent } - })); + }); @@ - ws.send(JSON.stringify({ + safeSend(ws, { type: 'session-created', sessionId: capturedSessionId - })); + }); @@ - ws.send(JSON.stringify({ + safeSend(ws, { type: 'gemini-error', error: errorMsg - })); + }); @@ - ws.send(JSON.stringify({ + safeSend(ws, { type: 'gemini-complete', exitCode: code, isNewSession: !sessionId && !!command // Flag to indicate this was a new session - })); + }); @@ - ws.send(JSON.stringify({ + safeSend(ws, { type: 'gemini-error', error: error.message - })); + });Also applies to: 278-284, 306-310, 328-331, 348-352, 383-386
src/components/ChatInterface.jsx (1)
1549-1566: Preserve streamed whitespace and avoid mutating React state in place (re-raising prior feedback).
- Gating with .trim() drops whitespace-only chunks, causing visible formatting issues with streaming.
- Mutating last.content in-place can be skipped by React.memo; use immutable updates.
- } else if (part.type === 'text' && part.text?.trim()) { + } else if (part.type === 'text' && typeof part.text === 'string' && part.text.length > 0) { // Append streaming text to the last assistant message if possible setChatMessages(prev => { const updated = [...prev]; const last = updated[updated.length - 1]; if (last && last.type === 'assistant' && !last.isToolUse) { - last.content += part.text; + updated[updated.length - 1] = { + ...last, + content: (last.content || '') + part.text + }; } else { updated.push({ type: 'assistant', content: part.text, timestamp: new Date() }); } return updated; }); maybeAutoScroll(); } @@ - } else if (typeof messageData.content === 'string' && messageData.content.trim()) { + } else if (typeof messageData.content === 'string' && messageData.content.length > 0) { // Append streaming text to the last assistant message if possible setChatMessages(prev => { const updated = [...prev]; const last = updated[updated.length - 1]; if (last && last.type === 'assistant' && !last.isToolUse) { - last.content += messageData.content; + updated[updated.length - 1] = { + ...last, + content: (last.content || '') + messageData.content + }; } else { updated.push({ type: 'assistant', content: messageData.content, timestamp: new Date() }); } return updated; }); maybeAutoScroll(); }Also applies to: 1569-1585
🧹 Nitpick comments (2)
server/gemini-cli.js (1)
241-274: Optional: bound memory growth in streaming accumulator.outputBuffer grows unbounded for long sessions. Consider tracking a processedRawLength pointer and slicing outputBuffer occasionally to keep only the trailing, not-yet-processed tail.
Also applies to: 252-267
src/components/ChatInterface.jsx (1)
2241-2246: Typo in class name prevents underline style.nderline → underline.
- className="ml-1 text-gemini-600 hover:text-gemini-800 nderline" + className="ml-1 text-gemini-600 hover:text-gemini-800 underline"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
server/gemini-cli.js(2 hunks)src/components/ChatInterface.jsx(12 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
server/gemini-cli.js (2)
server/projects.js (2)
lines(441-441)filteredLines(453-460)server/routes/mcp.js (2)
lines(221-221)lines(266-266)
src/components/ChatInterface.jsx (1)
src/App.jsx (1)
autoScrollToBottom(64-67)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (2)
src/components/ChatInterface.jsx (2)
1298-1318: Auto-scroll pause/resume logic looks good.isAtBottom thresholding and handleScroll toggling isAutoScrollPaused are correct; pairs well with scrollToBottom resetting the pause flag.
1821-1825: Nice UX touches for scrolling.
- Resetting isAutoScrollPaused on initial load and after programmatic scroll keeps auto-scroll predictable.
- The “scroll to bottom” affordance gated on isAutoScrollPaused is a good, unobtrusive control.
Also applies to: 1292-1296, 2323-2333
| const maybeAutoScroll = useCallback(() => { | ||
| if (autoScrollToBottom && !isUserScrolledUp) { | ||
| requestAnimationFrame(() => scrollToBottom()); | ||
| } | ||
| }, [isNearBottom]); | ||
| }, [autoScrollToBottom, isUserScrolledUp, scrollToBottom]); | ||
|
|
There was a problem hiding this comment.
Fix ReferenceError: maybeAutoScroll uses undefined isUserScrolledUp; should use isAutoScrollPaused.
This breaks auto-scroll logic whenever maybeAutoScroll is invoked.
- const maybeAutoScroll = useCallback(() => {
- if (autoScrollToBottom && !isUserScrolledUp) {
- requestAnimationFrame(() => scrollToBottom());
- }
- }, [autoScrollToBottom, isUserScrolledUp, scrollToBottom]);
+ const maybeAutoScroll = useCallback(() => {
+ if (autoScrollToBottom && !isAutoScrollPaused) {
+ requestAnimationFrame(() => scrollToBottom());
+ }
+ }, [autoScrollToBottom, isAutoScrollPaused, scrollToBottom]);Also applies to: 1091-1095
🤖 Prompt for AI Agents
In src/components/ChatInterface.jsx around lines 1321-1326 (and also at
1091-1095), maybeAutoScroll references the undefined variable isUserScrolledUp;
replace it with the correct state isAutoScrollPaused and invert the check so the
guard becomes autoScrollToBottom && !isAutoScrollPaused. Update the useCallback
dependency array to include isAutoScrollPaused instead of isUserScrolledUp (and
ensure scrollToBottom and autoScrollToBottom remain). Apply the same replacement
at the other occurrence (lines 1091-1095) so both callbacks use the correct
variable and dependencies.
qqq-tech
force-pushed
the
main
branch
2 times, most recently
from
cb35848 to
1ef41db
Compare
…-wrapper Wait for Gemini CLI process to fully exit before starting another
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
server/gemini-cli.js (3)
224-226: Interactive mode broken: stdin is closed unconditionallyYou close stdin before checking interactive mode, so interactive sessions can’t accept input.
- // Close stdin to signal we're done sending input - geminiProcess.stdin.end(); + // Defer stdin handling to the block below (depends on whether we have an initial prompt)Also applies to: 390-397
228-240: One-shot timeout clears on first chunk; add idle watchdog to avoid hangsAfter first stdout chunk you clearTimeout and never re-arm; a half-produced response can hang forever.
- let hasReceivedOutput = false; - const timeoutMs = 30000; // 30 seconds - const timeout = setTimeout(() => { - if (!hasReceivedOutput) { - // console.error('⏰ Gemini CLI timeout - no output received after', timeoutMs, 'ms'); - safeSend(ws, { - type: 'gemini-error', - error: 'Gemini CLI timeout - no response received' - }); - geminiProcess.kill('SIGTERM'); - } - }, timeoutMs); + let hasReceivedOutput = false; + const idleMs = 30000; // 30s idle timeout + let idleTimer = setTimeout(onIdle, idleMs); + function bumpIdle() { + clearTimeout(idleTimer); + idleTimer = setTimeout(onIdle, idleMs); + } + function onIdle() { + if (!hasReceivedOutput) { + safeSend(ws, { type: 'gemini-error', error: 'Gemini CLI timeout - no response received' }); + } else { + safeSend(ws, { type: 'gemini-error', error: 'Gemini CLI idle timeout' }); + } + try { geminiProcess.kill('SIGTERM'); } catch {} + }- hasReceivedOutput = true; - clearTimeout(timeout); + hasReceivedOutput = true; + bumpIdle();Remember to replace clearTimeout(timeout) in the 'close' handler with clearTimeout(idleTimer).
Also applies to: 250-252
61-63: Constrain temp image handling: use mkdtemp, cap count/size, and validate mimeAvoid unbounded disk writes and directory races.
- // Create temp directory in the project directory so Gemini can access it - tempDir = path.join(workingDir, '.tmp', 'images', Date.now().toString()); - await fs.mkdir(tempDir, { recursive: true }); + // Create a unique temp directory under the project + const baseTmp = path.join(workingDir, '.tmp', 'images'); + await fs.mkdir(baseTmp, { recursive: true }); + tempDir = await fs.mkdtemp(path.join(baseTmp, 'gemini-')); - // Save each image to a temp file - for (const [index, image] of images.entries()) { + const MAX_IMG_COUNT = Number(process.env.MAX_GEMINI_IMG_COUNT || 8); + const MAX_IMG_BYTES = Number(process.env.MAX_GEMINI_IMG_BYTES || 10 * 1024 * 1024); + // Save each image to a temp file + for (const [index, image] of images.entries()) { + if (index >= MAX_IMG_COUNT) break; // Extract base64 data and mime type const matches = image.data.match(/^data:([^;]+);base64,(.+)$/); if (!matches) { // console.error('Invalid image data format'); continue; } - const [, mimeType, base64Data] = matches; - const extension = mimeType.split('/')[1] || 'png'; + const [, mimeType, base64Data] = matches; + if (!/^image\/[a-z0-9.+-]+$/i.test(mimeType)) continue; + const approxBytes = Buffer.byteLength(base64Data, 'base64'); + if (approxBytes > MAX_IMG_BYTES) continue; + const subtype = (mimeType.split('/')[1] || 'png').toLowerCase(); + const extension = subtype.replace(/[^a-z0-9.+-]/gi, '') || 'png'; const filename = `image_${index}.${extension}`; const filepath = path.join(tempDir, filename); // Write base64 data to file await fs.writeFile(filepath, Buffer.from(base64Data, 'base64')); tempImagePaths.push(filepath); }Also applies to: 65-81
♻️ Duplicate comments (1)
server/gemini-cli.js (1)
233-237: Prevent crashes when ws is closed: use a safeSend wrapper for all emitsDirect ws.send can throw if the client disconnects mid-stream; this can crash the handler.
Apply replacements and add a helper:
- ws.send(JSON.stringify({ + safeSend(ws, { type: 'gemini-error', error: 'Gemini CLI timeout - no response received' - })); + });- ws.send(JSON.stringify({ + safeSend(ws, { type: 'gemini-response', data: { type: 'message', content: filteredOutput } - })); + });- ws.send(JSON.stringify({ + safeSend(ws, { type: 'session-created', sessionId: capturedSessionId - })); + });- ws.send(JSON.stringify({ + safeSend(ws, { type: 'gemini-error', error: errorMsg - })); + });- ws.send(JSON.stringify({ + safeSend(ws, { type: 'gemini-complete', exitCode: code, isNewSession: !sessionId && !!command - })); + });- ws.send(JSON.stringify({ + safeSend(ws, { type: 'gemini-error', error: error.message - })); + });Add once near the top (outside the ranges):
function safeSend(ws, payload) { try { if (ws && ws.readyState === 1 /* OPEN */) { ws.send(JSON.stringify(payload)); } } catch (_) { // client likely disconnected; ignore } }Also applies to: 276-283, 304-307, 325-330, 346-351, 381-385
🧹 Nitpick comments (5)
server/gemini-cli.js (5)
86-96: Hard-coded Japanese in prompt note; use English or i18nThe embedded note may confuse English users.
- const imageNote = `\n\n[画像を添付しました: ${tempImagePaths.length}枚の画像があります。以下のパスに保存されています:]\n${tempImagePaths.map((p, i) => `${i + 1}. ${p}`).join('\n')}`; + const imageNote = `\n\n[Attached ${tempImagePaths.length} image(s). Saved at paths:]\n${tempImagePaths.map((p, i) => `${i + 1}. ${p}`).join('\n')}`;
9-16: API change: function now returns a Promise (non-async declaration)Callers relying on an async function type should be unaffected, but double-check any typings.
181-183: Default model fallbackGood to have a default; consider making it a constant or env-driven to avoid hardcoding.
- const modelToUse = model || 'gemini-2.5-flash'; + const modelToUse = model || process.env.GEMINI_DEFAULT_MODEL || 'gemini-2.5-flash';
457-494: getGeminiSpec parityPromise-based refactor looks fine. Consider mirroring the same idle-timeout pattern to avoid hung specs.
119-166: Unify config file locations
server/gemini-cli.js reads~/.gemini.jsonwhile server/projects.js reads~/.gemini/project-config.json; consolidate both under a single config directory (e.g.~/.gemini/settings.json) or add support for both paths.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/gemini-cli.js(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
server/gemini-cli.js (3)
server/projects.js (2)
configPath(18-18)configPath(30-30)server/routes/mcp.js (4)
process(24-26)process(96-98)process(139-141)process(182-184)server/index.js (1)
fs(771-771)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (1)
server/gemini-cli.js (1)
355-362: Temp file cleanup already resilientNon-fatal cleanup errors are swallowed intentionally; looks good.
| return new Promise((resolve) => { | ||
| // Try to find the process by session ID or any key that contains the session ID | ||
| let process = activeGeminiProcesses.get(sessionId); | ||
| let processKey = sessionId; | ||
|
|
||
| if (!process) { | ||
| for (const [key, proc] of activeGeminiProcesses.entries()) { | ||
| if (key.includes(sessionId) || sessionId.includes(key)) { | ||
| process = proc; | ||
| processKey = key; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (process) { | ||
| // Debug - Found process for session | ||
|
|
||
| if (!process) { | ||
| resolve(false); | ||
| return; | ||
| } | ||
|
|
||
| const cleanup = () => { | ||
| activeGeminiProcesses.delete(processKey); | ||
| resolve(true); | ||
| }; | ||
|
|
||
| process.once('close', cleanup); | ||
| process.once('exit', cleanup); | ||
|
|
||
| try { | ||
| // First try SIGTERM | ||
| process.kill('SIGTERM'); | ||
|
|
||
| // Set a timeout to force kill if process doesn't exit | ||
| setTimeout(() => { | ||
| if (activeGeminiProcesses.has(processKey)) { | ||
| // Debug - Process didn't terminate, forcing kill | ||
| try { | ||
| process.kill('SIGKILL'); | ||
| } catch (e) { | ||
| // console.error('Error force killing process:', e); | ||
| } | ||
| } | ||
| }, 2000); // Wait 2 seconds before force kill | ||
|
|
||
| activeGeminiProcesses.delete(processKey); | ||
| return true; | ||
| } catch (error) { | ||
| // console.error('Error killing process:', error); | ||
| activeGeminiProcesses.delete(processKey); | ||
| return false; | ||
| } catch { | ||
| cleanup(); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| // Debug - No process found for session | ||
| return false; | ||
|
|
||
| // Force kill if process doesn't exit | ||
| setTimeout(() => { | ||
| if (activeGeminiProcesses.has(processKey)) { | ||
| try { | ||
| process.kill('SIGKILL'); | ||
| } catch { | ||
| // ignore | ||
| } | ||
| } | ||
| }, 2000); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
abortGeminiSession can hang; also shadows global process
If the child ignores SIGTERM/SIGKILL or events don’t fire, the Promise may never resolve. Also, naming the child “process” shadows Node’s global process.
- return new Promise((resolve) => {
+ return new Promise((resolve) => {
- let process = activeGeminiProcesses.get(sessionId);
- let processKey = sessionId;
+ let child = activeGeminiProcesses.get(sessionId);
+ let processKey = sessionId;
- if (!process) {
- for (const [key, proc] of activeGeminiProcesses.entries()) {
+ if (!child) {
+ for (const [key, proc] of activeGeminiProcesses.entries()) {
if (key.includes(sessionId) || sessionId.includes(key)) {
- process = proc;
+ child = proc;
processKey = key;
break;
}
}
}
- if (!process) {
+ if (!child) {
resolve(false);
return;
}
- const cleanup = () => {
- activeGeminiProcesses.delete(processKey);
- resolve(true);
- };
+ let done = false;
+ const finalize = (ok) => {
+ if (done) return;
+ done = true;
+ activeGeminiProcesses.delete(processKey);
+ resolve(ok);
+ };
- process.once('close', cleanup);
- process.once('exit', cleanup);
+ child.once('close', () => finalize(true));
+ child.once('exit', () => finalize(true));
try {
- process.kill('SIGTERM');
- } catch {
- cleanup();
+ child.kill('SIGTERM');
+ } catch {
+ finalize(false);
return;
}
- // Force kill if process doesn't exit
- setTimeout(() => {
- if (activeGeminiProcesses.has(processKey)) {
- try {
- process.kill('SIGKILL');
- } catch {
- // ignore
- }
- }
- }, 2000);
+ // Force kill if process doesn't exit, then hard-timeout the wait
+ setTimeout(() => {
+ if (activeGeminiProcesses.has(processKey)) {
+ try { child.kill('SIGKILL'); } catch {}
+ }
+ // Hard stop after 5s total
+ setTimeout(() => finalize(false), 3000);
+ }, 2000);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return new Promise((resolve) => { | |
| // Try to find the process by session ID or any key that contains the session ID | |
| let process = activeGeminiProcesses.get(sessionId); | |
| let processKey = sessionId; | |
| if (!process) { | |
| for (const [key, proc] of activeGeminiProcesses.entries()) { | |
| if (key.includes(sessionId) || sessionId.includes(key)) { | |
| process = proc; | |
| processKey = key; | |
| break; | |
| } | |
| } | |
| } | |
| } | |
| if (process) { | |
| // Debug - Found process for session | |
| if (!process) { | |
| resolve(false); | |
| return; | |
| } | |
| const cleanup = () => { | |
| activeGeminiProcesses.delete(processKey); | |
| resolve(true); | |
| }; | |
| process.once('close', cleanup); | |
| process.once('exit', cleanup); | |
| try { | |
| // First try SIGTERM | |
| process.kill('SIGTERM'); | |
| // Set a timeout to force kill if process doesn't exit | |
| setTimeout(() => { | |
| if (activeGeminiProcesses.has(processKey)) { | |
| // Debug - Process didn't terminate, forcing kill | |
| try { | |
| process.kill('SIGKILL'); | |
| } catch (e) { | |
| // console.error('Error force killing process:', e); | |
| } | |
| } | |
| }, 2000); // Wait 2 seconds before force kill | |
| activeGeminiProcesses.delete(processKey); | |
| return true; | |
| } catch (error) { | |
| // console.error('Error killing process:', error); | |
| activeGeminiProcesses.delete(processKey); | |
| return false; | |
| } catch { | |
| cleanup(); | |
| return; | |
| } | |
| } | |
| // Debug - No process found for session | |
| return false; | |
| // Force kill if process doesn't exit | |
| setTimeout(() => { | |
| if (activeGeminiProcesses.has(processKey)) { | |
| try { | |
| process.kill('SIGKILL'); | |
| } catch { | |
| // ignore | |
| } | |
| } | |
| }, 2000); | |
| }); | |
| } | |
| return new Promise((resolve) => { | |
| // Try to find the process by session ID or any key that contains the session ID | |
| let child = activeGeminiProcesses.get(sessionId); | |
| let processKey = sessionId; | |
| if (!child) { | |
| for (const [key, proc] of activeGeminiProcesses.entries()) { | |
| if (key.includes(sessionId) || sessionId.includes(key)) { | |
| child = proc; | |
| processKey = key; | |
| break; | |
| } | |
| } | |
| } | |
| if (!child) { | |
| resolve(false); | |
| return; | |
| } | |
| let done = false; | |
| const finalize = (ok) => { | |
| if (done) return; | |
| done = true; | |
| activeGeminiProcesses.delete(processKey); | |
| resolve(ok); | |
| }; | |
| child.once('close', () => finalize(true)); | |
| child.once('exit', () => finalize(true)); | |
| try { | |
| child.kill('SIGTERM'); | |
| } catch { | |
| finalize(false); | |
| return; | |
| } | |
| // Force kill if process doesn't exit, then hard-timeout the wait | |
| setTimeout(() => { | |
| if (activeGeminiProcesses.has(processKey)) { | |
| try { child.kill('SIGKILL'); } catch {} | |
| } | |
| // Hard stop after 5s total | |
| setTimeout(() => finalize(false), 3000); | |
| }, 2000); | |
| }); | |
| } |
🤖 Prompt for AI Agents
In server/gemini-cli.js around lines 403 to 449, the function can hang and also
shadows Node’s global process variable: rename the local variable (e.g., child
or proc) instead of "process", add a single-resolution guard (resolved boolean)
and ensure all listeners are removed when finishing, and guarantee the Promise
resolves on a timeout by invoking cleanup (and attempting SIGKILL) if the child
hasn't exited within the deadline; also wrap kill calls in try/catch and ensure
cleanup deletes from activeGeminiProcesses, removes listeners, and resolves only
once.
…esponse fix: avoid timestamps in copied chat text
…lines-in-response Revert "fix: avoid timestamps in copied chat text"
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/components/ChatInterface.jsx (1)
1538-1545: Whitespace-only streamed chunks are still dropped (regression)trim() filters out chunks that are just whitespace, causing formatting loss during streaming. Prior review flagged this.
- } else if (part.type === 'text' && part.text?.trim()) { + } else if (part.type === 'text' && typeof part.text === 'string' && part.text.length > 0) { // Add regular text message setChatMessages(prev => [...prev, { type: 'assistant', content: part.text, timestamp: new Date() }]); } ... - } else if (typeof messageData.content === 'string' && messageData.content.trim()) { + } else if (typeof messageData.content === 'string' && messageData.content.length > 0) { // Add regular text message setChatMessages(prev => [...prev, { type: 'assistant', content: messageData.content, timestamp: new Date() }]); }Also applies to: 1548-1554
🧹 Nitpick comments (4)
src/components/ui/ToolUseFeedback.jsx (1)
298-303: Use semantic with dateTime; keep it non-interactiveSwap the div for a time element to add machine-readable semantics without changing visual/AT behavior.
Confirm that hiding timestamps from screen readers is intended.
- <div - className="text-xs text-zinc-500 dark:text-zinc-400 select-none pointer-events-none" - aria-hidden="true" - > - {timestampText} - </div> + <time + className="text-xs text-zinc-500 dark:text-zinc-400 select-none pointer-events-none" + dateTime={new Date(toolResultTimestamp || Date.now()).toISOString()} + aria-hidden="true" + > + {timestampText} + </time>src/components/ChatInterface.jsx (3)
100-105: Prefer with dateTime for timestampsAdds semantics; still hidden and non-interactive as intended.
- <div - className="text-xs text-gemini-100 mt-1 text-right select-none pointer-events-none" - aria-hidden="true" - > - {new Date(message.timestamp).toLocaleTimeString()} - </div> + <time + className="text-xs text-gemini-100 mt-1 text-right select-none pointer-events-none" + dateTime={new Date(message.timestamp).toISOString()} + aria-hidden="true" + > + {new Date(message.timestamp).toLocaleTimeString()} + </time>
988-993: Same semantic improvement for assistant/Gemini timestampMirror the change with a time element.
- <div - className={`text-xs text-gray-500 dark:text-gray-400 mt-1 ${isGrouped ? 'opacity-0 group-hover:opacity-100' : ''} select-none pointer-events-none`} - aria-hidden="true" - > - {new Date(message.timestamp).toLocaleTimeString()} - </div> + <time + className={`text-xs text-gray-500 dark:text-gray-400 mt-1 ${isGrouped ? 'opacity-0 group-hover:opacity-100' : ''} select-none pointer-events-none`} + dateTime={new Date(message.timestamp).toISOString()} + aria-hidden="true" + > + {new Date(message.timestamp).toLocaleTimeString()} + </time>
2233-2239: Minor typo in className (“nderline” → “underline”)Visual nit; fixes missing underline on “Load earlier messages”.
- <button - className="ml-1 text-gemini-600 hover:text-gemini-800 nderline" + <button + className="ml-1 text-gemini-600 hover:text-gemini-800 underline"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/ChatInterface.jsx(2 hunks)src/components/ui/ToolUseFeedback.jsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Style