feat: add DeepSeek models, fix Shell tab switching, add image/PDF preview#742
Draft
z2512690268 wants to merge 2 commits intositeboon:mainfrom
Draft
feat: add DeepSeek models, fix Shell tab switching, add image/PDF preview#742z2512690268 wants to merge 2 commits intositeboon:mainfrom
z2512690268 wants to merge 2 commits intositeboon:mainfrom
Conversation
…view - Add deepseek-v4-flash and deepseek-v4-pro to model selector - Fix Shell→Chat→Shell switching: keep Shell mounted with absolute positioning instead of unmounting, preventing PTY teardown - Fix PTY onExit race condition when old PTY is killed and new one is spawned - Add image and PDF preview in code editor sidebar/fullscreen - Fix xterm.js layout corruption when Shell tab becomes visible after being hidden Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughServer-side shell websocket now always replaces prior PTY on reconnect and guards stdout/exit handlers; client-side adds file category detection with image/pdf previews, new image/pdf preview components, main-content overlay layout changes, and shell terminal fit/teardown improvements. Two Claude model options were added. ChangesShell WebSocket Session Management (server)
File Category & Preview System (client)
Claude Model Options
Shell Display & Overlay Infrastructure (client)
Sequence DiagramsequenceDiagram
participant Client as Client (WebSocket)
participant Server as Server (WebSocket handler)
participant PTY as PTY process
Note over Client,Server: Reconnect sequence (new behavior)
Client->>Server: open websocket (init, sessionKey)
Server->>Server: lookup session map by sessionKey
alt existing session present
Server->>PTY: clear session timeout
Server->>PTY: attempt to kill prior PTY (best-effort)
Server->>Server: delete session map entry
end
Server->>PTY: spawn new PTY
PTY-->>Server: onData events (bound to spawned PTY)
Server->>Client: relay PTY output
PTY-->>Server: onExit (only acts if session still references spawned PTY)
Server->>Client: send exit/cleanup messages
Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/modules/websocket/services/shell-websocket.service.ts (1)
268-275:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: stale PTY callbacks race because they read mutable outer bindings.
At lines 273–274 and 349–352, callbacks read
ptySessionKeyandshellProcessfrom mutable outer scope. When a subsequentinitcall reassigns these variables to new values, old PTY handlers can evaluate against the new PTY and incorrectly pass the identity guard, causing stale output emission or deletion of the active session.Proposed fix: capture immutable key/process per spawned PTY
- ptySessionKey = `${projectPath}_${sessionId ?? 'default'}${commandSuffix}`; + const sessionKey = `${projectPath}_${sessionId ?? 'default'}${commandSuffix}`; + ptySessionKey = sessionKey; ... - shellProcess = pty.spawn(shell, shellArgs, { + const spawnedPty = pty.spawn(shell, shellArgs, { name: 'xterm-256color', cols: termCols, rows: termRows, cwd: resolvedProjectPath, env: { ...process.env, TERM: 'xterm-256color', COLORTERM: 'truecolor', FORCE_COLOR: '3', }, }); + shellProcess = spawnedPty; - ptySessionsMap.set(ptySessionKey, { - pty: shellProcess, + ptySessionsMap.set(sessionKey, { + pty: spawnedPty, ws, buffer: [], timeoutId: null, projectPath, sessionId, }); - shellProcess.onData((chunk) => { - if (!ptySessionKey) { - return; - } - - const session = ptySessionsMap.get(ptySessionKey); - if (!session || session.pty !== shellProcess) { + spawnedPty.onData((chunk) => { + const session = ptySessionsMap.get(sessionKey); + if (!session || session.pty !== spawnedPty) { return; } ... - shellProcess.onExit((exitCode) => { - if (!ptySessionKey) { - return; - } - - const session = ptySessionsMap.get(ptySessionKey); + spawnedPty.onExit((exitCode) => { + const session = ptySessionsMap.get(sessionKey); // Only act if this PTY is still the active one — a replacement PTY // may have been spawned while this one was being killed. - if (!session || session.pty !== shellProcess) { + if (!session || session.pty !== spawnedPty) { return; } ... - ptySessionsMap.delete(ptySessionKey); - shellProcess = null; + ptySessionsMap.delete(sessionKey); + if (shellProcess === spawnedPty) { + shellProcess = null; + } });Also applies to: 344–354
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/modules/websocket/services/shell-websocket.service.ts` around lines 268 - 275, The callbacks for shellProcess (shellProcess.onData and shellProcess.onExit) are closing over mutable outer variables ptySessionKey and shellProcess, causing race conditions when init/spawn reassigns them; fix by capturing immutable locals inside the spawn/init scope (e.g., const localPtySessionKey = ptySessionKey and const localShell = shellProcess) and use those in the onData/onExit handlers and when calling ptySessionsMap.get, ptySessionsMap.delete, and any identity checks so each PTY's handlers always reference the correct process/key and removal uses the same captured references.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shared/modelConstants.js`:
- Around line 23-24: The DeepSeek entries were incorrectly placed in
CLAUDE_MODELS causing provider selection to stay "claude" and triggering
queryClaudeSDK with invalid models; fix by removing the DeepSeek entries from
CLAUDE_MODELS and adding a new DEEPSEEK_MODELS constant (with the
deepseek-v4-flash and deepseek-v4-pro values), add a corresponding PROVIDERS
entry for provider id "deepseek" and implement/route to a queryDeepSeekSDK
handler (or extend the provider dispatch logic) so the agent route handler
selects the "deepseek" provider instead of calling queryClaudeSDK for DeepSeek
models.
In `@src/components/code-editor/view/CodeEditor.tsx`:
- Around line 169-190: The image/pdf preview branches pass the original file
object which can lack projectId (the preview components use file.projectId ??
''), so preserve the fallback by forwarding a file with projectId populated from
projectPath when missing; update the CodeEditorImagePreview and
CodeEditorPdfPreview calls to pass {...file, projectId: file.projectId ??
file.projectPath ?? ''} so the previews request the correct project id.
In `@src/components/code-editor/view/subcomponents/CodeEditorPdfPreview.tsx`:
- Around line 93-103: The close (and fullscreen) icon-only buttons in
CodeEditorPdfPreview.tsx lack explicit accessible names; add aria-label
attributes (e.g., aria-label="Close" for the button that calls onClose and
aria-label="Toggle fullscreen" or similar for the fullscreen control) to ensure
screen readers get a reliable name, and mirror the same changes in
CodeEditorImagePreview.tsx for the equivalent buttons; leave the existing title
prop if desired but ensure aria-label is present on the <button> elements that
use onClose and the fullscreen handler.
---
Outside diff comments:
In `@server/modules/websocket/services/shell-websocket.service.ts`:
- Around line 268-275: The callbacks for shellProcess (shellProcess.onData and
shellProcess.onExit) are closing over mutable outer variables ptySessionKey and
shellProcess, causing race conditions when init/spawn reassigns them; fix by
capturing immutable locals inside the spawn/init scope (e.g., const
localPtySessionKey = ptySessionKey and const localShell = shellProcess) and use
those in the onData/onExit handlers and when calling ptySessionsMap.get,
ptySessionsMap.delete, and any identity checks so each PTY's handlers always
reference the correct process/key and removal uses the same captured references.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 80f9297e-56c1-4cb9-a510-a8109ad62538
📒 Files selected for processing (11)
server/modules/websocket/services/shell-websocket.service.tsshared/modelConstants.jssrc/components/code-editor/hooks/useCodeEditorDocument.tssrc/components/code-editor/utils/binaryFile.tssrc/components/code-editor/view/CodeEditor.tsxsrc/components/code-editor/view/subcomponents/CodeEditorImagePreview.tsxsrc/components/code-editor/view/subcomponents/CodeEditorPdfPreview.tsxsrc/components/main-content/view/MainContent.tsxsrc/components/shell/hooks/useShellRuntime.tssrc/components/shell/types/types.tssrc/components/shell/view/Shell.tsx
Comment on lines
+23
to
+24
| { value: "deepseek-v4-flash", label: "DeepSeek V4 Flash" }, | ||
| { value: "deepseek-v4-pro", label: "DeepSeek V4 Pro" }, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify how CLAUDE_MODELS / PROVIDERS are consumed downstream to determine
# if DeepSeek entries being under the "claude" provider causes routing failures.
# Find all files that import or reference CLAUDE_MODELS or PROVIDERS
rg -n --type=ts --type=js -C3 'CLAUDE_MODELS|PROVIDERS' \
-g '!shared/modelConstants.js'Repository: siteboon/claudecodeui
Length of output: 11976
🏁 Script executed:
#!/bin/bash
# Search for API endpoint/base URL configuration and how it's determined by provider
rg -n --type=js --type=ts 'endpoint|baseUrl|BASE_URL|api\.anthropic|api\.deepseek' \
-A2 -B2 | head -100Repository: siteboon/claudecodeui
Length of output: 5764
🏁 Script executed:
#!/bin/bash
# Search for any DeepSeek-specific handling in backend
rg -n --type=js --type=ts -i 'deepseek' \
server/ src/Repository: siteboon/claudecodeui
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Check claude-sdk.js to see how it's initialized and whether it uses provider-specific config
head -80 server/claude-sdk.jsRepository: siteboon/claudecodeui
Length of output: 2539
🏁 Script executed:
#!/bin/bash
# Search for where backend routes based on provider to different SDK handlers
rg -n --type=js 'provider.*===.*claude|if.*provider' server/routes/ -A3 -B1Repository: siteboon/claudecodeui
Length of output: 3477
🏁 Script executed:
#!/bin/bash
# Look for where queryClaude, queryGemini, queryCodex functions are called
rg -n --type=js 'queryClaude|queryGemini|queryCodex|spawnGemini' server/ -B5 -A2 | head -150Repository: siteboon/claudecodeui
Length of output: 7769
🏁 Script executed:
#!/bin/bash
# Check agent.js to see the main dispatch logic
cat -n server/routes/agent.js | head -100Repository: siteboon/claudecodeui
Length of output: 3921
🏁 Script executed:
#!/bin/bash
# Check queryClaudeSDK to see if there's any endpoint override based on model name
sed -n '200,250p' server/claude-sdk.jsRepository: siteboon/claudecodeui
Length of output: 1618
🏁 Script executed:
#!/bin/bash
# Search for any endpoint/baseUrl configuration in claude-sdk.js
rg -n 'baseUrl|BASE_URL|endpoint|api\.anthropic' server/claude-sdk.js -B2 -A2Repository: siteboon/claudecodeui
Length of output: 309
DeepSeek models misclassified under CLAUDE_MODELS — will cause runtime failures
CLAUDE_MODELS is explicitly the "Claude (Anthropic) Models" section, and the backend's provider dispatch logic (server/routes/agent.js:942) uses hardcoded if/else on provider ID. When a user selects a DeepSeek model from the frontend, the provider remains "claude", triggering queryClaudeSDK() at runtime.
The queryClaudeSDK() function (server/claude-sdk.js:207) passes the model value directly to the Anthropic SDK without any endpoint override or model-based routing. This means a request with model="deepseek-v4-flash" will be sent to the Anthropic API, which will reject it as an invalid model, causing the request to fail.
These models require either a dedicated DEEPSEEK_MODELS constant with a corresponding PROVIDERS entry and backend handler, or routing logic that intercepts DeepSeek model names and redirects them to the correct endpoint and API client.
🤖 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 `@shared/modelConstants.js` around lines 23 - 24, The DeepSeek entries were
incorrectly placed in CLAUDE_MODELS causing provider selection to stay "claude"
and triggering queryClaudeSDK with invalid models; fix by removing the DeepSeek
entries from CLAUDE_MODELS and adding a new DEEPSEEK_MODELS constant (with the
deepseek-v4-flash and deepseek-v4-pro values), add a corresponding PROVIDERS
entry for provider id "deepseek" and implement/route to a queryDeepSeekSDK
handler (or extend the provider dispatch logic) so the agent route handler
selects the "deepseek" provider instead of calling queryClaudeSDK for DeepSeek
models.
…iolation The previewFile useMemo was placed after the if (loading) early return, causing a white screen on file open. Moved it before the loading check. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/code-editor/view/subcomponents/CodeEditorImagePreview.tsx`:
- Around line 30-47: The imageUrl state isn't cleared when a new load starts or
when loading fails, causing the UI to show a stale <img> alongside an error;
update the CodeEditorImagePreview component to call setImageUrl(null) (or '') at
the start of the load (near setLoading(true); setError(null);) and again inside
the catch block before setError('Unable to load image'), and ensure any previous
objectUrl is revoked (URL.revokeObjectURL) when replacing/clearing it to avoid
leaks; reference the setImageUrl, setLoading, setError calls and the objectUrl
variable when making these changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: dfaf9be2-0d6e-43a4-a3c5-647da27d1bc9
📒 Files selected for processing (4)
server/modules/websocket/services/shell-websocket.service.tssrc/components/code-editor/view/CodeEditor.tsxsrc/components/code-editor/view/subcomponents/CodeEditorImagePreview.tsxsrc/components/code-editor/view/subcomponents/CodeEditorPdfPreview.tsx
✅ Files skipped from review due to trivial changes (1)
- src/components/code-editor/view/subcomponents/CodeEditorPdfPreview.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/code-editor/view/CodeEditor.tsx
- server/modules/websocket/services/shell-websocket.service.ts
Comment on lines
+30
to
+47
| setLoading(true); | ||
| setError(null); | ||
|
|
||
| const response = await api.readFileBlob(file.projectId ?? '', file.path); | ||
|
|
||
| if (!response.ok) { | ||
| throw new Error(`Request failed with status ${response.status}`); | ||
| } | ||
|
|
||
| const blob = await response.blob(); | ||
| if (cancelled) return; | ||
| objectUrl = URL.createObjectURL(blob); | ||
| setImageUrl(objectUrl); | ||
| } catch (err: unknown) { | ||
| if (cancelled) return; | ||
| console.error('Error loading image:', err); | ||
| setError('Unable to load image'); | ||
| } finally { |
Contributor
There was a problem hiding this comment.
Clear imageUrl on reload/failure to avoid mixed success/error UI.
imageUrl is not reset when a new load starts (Line 30) or when load fails (Line 43). If a previous URL remains truthy, Line 68 can still render <img> while Line 75 renders the error block.
Proposed fix
const loadImage = async () => {
try {
setLoading(true);
setError(null);
+ setImageUrl(null);
const response = await api.readFileBlob(file.projectId ?? '', file.path);
@@
} catch (err: unknown) {
if (cancelled) return;
console.error('Error loading image:', err);
+ setImageUrl(null);
setError('Unable to load image');
} finally {
if (!cancelled) setLoading(false);
}
};Also applies to: 68-77
🤖 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/code-editor/view/subcomponents/CodeEditorImagePreview.tsx`
around lines 30 - 47, The imageUrl state isn't cleared when a new load starts or
when loading fails, causing the UI to show a stale <img> alongside an error;
update the CodeEditorImagePreview component to call setImageUrl(null) (or '') at
the start of the load (near setLoading(true); setError(null);) and again inside
the catch block before setError('Unable to load image'), and ensure any previous
objectUrl is revoked (URL.revokeObjectURL) when replacing/clearing it to avoid
leaks; reference the setImageUrl, setLoading, setError calls and the objectUrl
variable when making these changes.
Collaborator
|
Hey @z2512690268, thanks for the PR! Please separate the work into different pieces and submit a pr for each one of them. For e.g. adding a deep seek model should be a separate PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
deepseek-v4-flashanddeepseek-v4-proto the model selectoronExitrace condition where a killed PTY's exit event would interfere with the replacement PTY session.<iframe>in the editor sidebar/fullscreen overlay.Changes
shared/modelConstants.jsserver/modules/websocket/services/shell-websocket.service.tssrc/components/main-content/view/MainContent.tsxsrc/components/shell/view/Shell.tsxsrc/components/shell/hooks/useShellRuntime.tsfitAddonReffor dimension recalculationsrc/components/shell/types/types.tsfitAddonRefto result typesrc/components/code-editor/utils/binaryFile.tssrc/components/code-editor/hooks/useCodeEditorDocument.tsfileCategoryfor preview routingsrc/components/code-editor/view/CodeEditor.tsxsrc/components/code-editor/view/subcomponents/CodeEditorImagePreview.tsxsrc/components/code-editor/view/subcomponents/CodeEditorPdfPreview.tsx🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements