fix: resolve chat input lag during active SSE streaming#490
fix: resolve chat input lag during active SSE streaming#490TheDarkSkyXD wants to merge 1 commit intospacedriveapp:mainfrom
Conversation
WalkthroughBoth Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Convert WebChatPanel and CortexChatPanel text inputs from controlled (useState) to uncontrolled (ref-based) textareas so keystrokes bypass React's render cycle entirely. Wrap input components and message lists in React.memo to prevent expensive re-renders of long chat histories on every SSE event. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
TheDarkSkyXD
force-pushed
the
fix/chat-input-lag
branch
from
2307d34 to
2e07d75
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/config/types.rs (1)
840-846:⚠️ Potential issue | 🟠 MajorKeep JavaScript evaluation opt-in by default.
Line 845 changes
evaluate_enabledtotrue, which enablesbrowser_evaluatefor any agent that doesn’t explicitly override config (viaDefaultsConfig+unwrap_or(base.evaluate_enabled)merge). That weakens least-privilege defaults and expands prompt-injection blast radius.🔧 Suggested fix
impl Default for BrowserConfig { fn default() -> Self { Self { enabled: true, headless: true, - evaluate_enabled: true, + evaluate_enabled: false, executable_path: None, screenshot_dir: None, persist_session: false, close_policy: ClosePolicy::default(), chrome_cache_dir: PathBuf::from("chrome_cache"),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/types.rs` around lines 840 - 846, The default for BrowserConfig enables JavaScript evaluation by setting evaluate_enabled to true; revert this to keep JS evaluation opt-in by default by changing BrowserConfig::default so evaluate_enabled is false and ensure any existing merge/override logic that uses DefaultsConfig and unwrap_or(base.evaluate_enabled) continues to respect explicit per-agent settings rather than flipping to true implicitly—update the BrowserConfig::default implementation (symbol: BrowserConfig, field: evaluate_enabled) accordingly.
🧹 Nitpick comments (5)
interface/src/components/CortexChatPanel.tsx (1)
181-189: Consider extractingadjustHeightinto a shared utility.Both
FloatingChatInput(WebChatPanel.tsx lines 108-116) andCortexChatInput(lines 181-189) have nearly identicaladjustHeightimplementations, differing only inmaxHeight(200 vs 160). This could be extracted into a shared utility or custom hook.✏️ Optional extraction example
Create a shared utility:
// utils/textarea.ts export function adjustTextareaHeight(textarea: HTMLTextAreaElement, maxHeight: number) { textarea.style.height = "auto"; const scrollHeight = textarea.scrollHeight; textarea.style.height = `${Math.min(scrollHeight, maxHeight)}px`; textarea.style.overflowY = scrollHeight > maxHeight ? "auto" : "hidden"; }Then use in both components:
const adjustHeight = () => { if (textareaRef.current) { adjustTextareaHeight(textareaRef.current, 160); // or 200 } };Also applies to: 108-116
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/components/CortexChatPanel.tsx` around lines 181 - 189, Both CortexChatInput's adjustHeight (in CortexChatPanel.tsx) and FloatingChatInput's adjustHeight (in WebChatPanel.tsx) duplicate textarea resizing logic; extract that logic into a shared utility (e.g., export function adjustTextareaHeight(textarea: HTMLTextAreaElement, maxHeight: number) in utils/textarea.ts or a custom hook useAutoResizeTextarea) and replace each component's adjustHeight with a thin wrapper that calls the shared function passing the appropriate maxHeight (160 for CortexChatInput, 200 for FloatingChatInput); update imports and ensure textareaRef.current null checks remain in the components.interface/src/components/WebChatPanel.tsx (1)
194-194: Consider usingReact.RefObject<HTMLDivElement>without| nullfor cleaner typing.In React 19,
RefObjectalready allowsnullfor thecurrentproperty. The typeReact.RefObject<HTMLDivElement | null>is slightly redundant, though it still works correctly.✏️ Optional type simplification
- messagesEndRef: React.RefObject<HTMLDivElement | null>; + messagesEndRef: React.RefObject<HTMLDivElement>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/components/WebChatPanel.tsx` at line 194, The property messagesEndRef is typed redundantly as React.RefObject<HTMLDivElement | null>; update its declaration to React.RefObject<HTMLDivElement> to rely on RefObject's built-in nullable current type (locate the messagesEndRef declaration in the WebChatPanel component and replace its type accordingly), ensuring any places that access messagesEndRef.current remain type-safe and adjust any explicit null assertions if present.scripts/bundle-sidecar.ps1 (2)
20-22: Potential failure ifrustcis not available or output parsing fails.If
rustcis not installed or theSelect-Stringreturns nothing, the script will fail with an unclear error. Consider adding validation.🛡️ Proposed defensive check
# Determine Rust target triple +$RustcOutput = rustc -vV +if ($LASTEXITCODE -ne 0) { + Write-Error "Failed to run rustc. Ensure Rust is installed." + exit 1 +} -$HostTriple = (rustc -vV | Select-String "^host:").ToString().Split(" ")[1] +$HostLine = $RustcOutput | Select-String "^host:" +if (-not $HostLine) { + Write-Error "Could not determine Rust host triple from rustc output." + exit 1 +} +$HostTriple = $HostLine.ToString().Split(" ")[1] $TargetTriple = if ($env:TAURI_ENV_TARGET_TRIPLE) { $env:TAURI_ENV_TARGET_TRIPLE } else { $HostTriple }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/bundle-sidecar.ps1` around lines 20 - 22, The script assumes rustc exists and that Select-String returns a match when computing $HostTriple, which can fail silently; add defensive checks: first verify rustc is in PATH (e.g., test running "rustc -vV" and capture non-zero/empty output) and if it fails emit a clear error and exit, then ensure Select-String produced a match before accessing Split — if no match, emit a descriptive error or fall back to a sensible default; finally keep the existing $TargetTriple assignment but only use the parsed $HostTriple when it was successfully validated. Refer to the symbols $HostTriple, $TargetTriple, rustc and Select-String when locating where to add these validations.
43-52: PSScriptAnalyzer flagsWrite-Hostusage — acceptable for build scripts.The static analysis tool warns against
Write-Hostbecause it bypasses output streams. However, for interactive build scripts intended for developer use,Write-Hostis appropriate for progress messages. You can suppress this warning or optionally switch toWrite-Informationif you want strict compliance.♻️ Optional: Use Write-Information instead
-Write-Host "Building spacebot ($BuildMode) for $TargetTriple..." +Write-Information "Building spacebot ($BuildMode) for $TargetTriple..." -InformationAction Continue ... -Write-Host "Copied $SrcBin -> $DestBin" -Write-Host "Sidecar binary ready." +Write-Information "Copied $SrcBin -> $DestBin" -InformationAction Continue +Write-Information "Sidecar binary ready." -InformationAction ContinueOr add a suppression comment at the top of the script:
`#Requires` -Version 5.1 # PSScriptAnalyzer: disable PSAvoidUsingWriteHost🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/bundle-sidecar.ps1` around lines 43 - 52, PSScriptAnalyzer flags the use of Write-Host in the bundle-sidecar script (occurrences of Write-Host "Copied $SrcBin -> $DestBin" and Write-Host "Sidecar binary ready."), so either replace those Write-Host calls with Write-Information to route messages through the standard output streams or add a PSScriptAnalyzer suppression at the top of the script to disable PSAvoidUsingWriteHost (e.g., add a comment to disable PSAvoidUsingWriteHost or the suggested Requires header); choose one approach and apply it consistently for the Write-Host usages in this script.interface/src/components/ModelSelect.tsx (1)
126-140: Precompute the flat-index lookup.Every row does
flatList.indexOf(model)during render. Since each highlight change rerenders the whole list, this makes navigation O(n²). A memoized id→index map keeps the hot path linear.Suggested refactor
const flatList = useMemo(() => { const items: ModelInfo[] = []; for (const p of sortedProviders) { for (const m of grouped[p]) { items.push(m); } } return items; }, [sortedProviders, grouped]); + + const flatIndexById = useMemo( + () => new Map(flatList.map((model, index) => [model.id, index])), + [flatList], + ); // Find display name for current value const selectedModel = useMemo( () => models.find((m) => m.id === value), [models, value], @@ {grouped[prov].map((model) => { - const flatIdx = flatList.indexOf(model); + const flatIdx = flatIndexById.get(model.id) ?? -1; const isHighlighted = flatIdx === highlightIndex; const isSelected = model.id === value;Also applies to: 314-315
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/components/ModelSelect.tsx` around lines 126 - 140, The current flatList useMemo builds an array (flatList) and render code repeatedly calls flatList.indexOf(model), causing O(n²) rerenders; create a memoized map from model id to index (e.g., idToIndex) using useMemo alongside flatList (dependent on sortedProviders and grouped) and replace all flatList.indexOf(model) calls with idToIndex[model.id] (also apply the same change where models/value are used at lines referenced around selectedModel and the other occurrence at 314-315); ensure the map is kept in sync by computing it from the same flatList source so lookups become O(1).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@interface/src/components/ModelSelect.tsx`:
- Around line 318-333: The option buttons currently call handleSelect(model.id)
only from the onMouseDown handler which prevents keyboard activation; keep the
existing onMouseDown behavior to suppress blur but remove the call to
handleSelect from onMouseDown and instead invoke handleSelect(model.id) from an
onClick handler on the same button element; update the button that uses
setHighlightIndex and onMouseEnter to add onClick={() => handleSelect(model.id)}
and ensure onMouseDown only calls e.preventDefault() to preserve blur
suppression.
- Around line 209-214: The ArrowUp key branch in the key handler (in
ModelSelect.tsx) only updates setHighlightIndex using flatList and doesn't open
the menu when the input is closed; mirror the ArrowDown behavior by checking the
closed state and calling the same open/menu-open path (the same function or
state toggle used by the ArrowDown branch) before updating setHighlightIndex so
the first ArrowUp press opens the menu and moves the highlight as expected.
Ensure you reference the same open/menu toggle used elsewhere in this component
so behavior is consistent with the ArrowDown branch.
In `@scripts/bundle-sidecar.ps1`:
- Around line 35-41: After each external cargo build invocation (the two
branches that run "cargo build" when TargetTriple vs HostTriple are compared),
check $LASTEXITCODE immediately and abort if non-zero; in practice, after the
cargo build calls referenced by CargoFlags, TargetTriple, HostTriple, BuildMode
and RepoRoot, inspect $LASTEXITCODE and call exit $LASTEXITCODE or throw an
error before continuing to set $SrcBin or perform any copy so the script does
not proceed when the build failed.
In `@src/agent/compactor.rs`:
- Around line 175-192: The boundary scan can miss the case when remove_count ==
total.saturating_sub(2), leaving an orphaned User ToolResult; update the loop in
compactor.rs that advances remove_count (the one using history.get(remove_count)
and matching Message::User / UserContent::ToolResult) to include the equality
case (e.g., use <= or remove the strict < bound) and, when the scan reaches
exactly total.saturating_sub(2), handle it by splitting the boundary User
message instead of keeping it whole (i.e., trim or split the User content so
ToolResult is not orphaned) — apply the same change to the analogous scan block
referenced around lines 226-247.
In `@src/agent/worker.rs`:
- Around line 749-770: The overflow-recovery loop can still leave an orphaned
User ToolResult when remove_count equals total.saturating_sub(2); change the
loop bounds so the scan includes that index. Specifically, adjust the loop
condition around remove_count/total (and the initial clamping if needed) so the
while allows checking when remove_count == total.saturating_sub(2) (e.g., use <=
instead of < or clamp differently) and keep the existing logic that increments
remove_count when history.get(remove_count) matches rig::message::Message::User
with UserContent::ToolResult; this mirrors the channel compactor fix so the
paired ToolResult is advanced past the removal boundary.
In `@src/api/models.rs`:
- Around line 341-345: The new branches that push "ollama", "nvidia", and
"github-copilot" into providers are never translated by direct_provider_mapping,
so catalog lookups fall back to "openrouter" and filters like
configured.contains(...) or provider=<name> fail; update direct_provider_mapping
to emit the corresponding provider IDs for Ollama, NVIDIA, and GitHub Copilot
(or add a mapping helper used by direct_provider_mapping) so that provider
strings produced by providers.push("ollama"/"nvidia"/"github-copilot") match
catalog entries and the configured.contains(...) / provider=<name> filters.
Ensure the mapping is symmetric with has_key(...) usage and update any tests or
usages of direct_provider_mapping to include the new IDs.
In `@src/tools/browser.rs`:
- Around line 2057-2067: CloseBrowser currently swallows all errors from
browser.close() while CloseTabs collects and returns errors; change CloseBrowser
to mirror CloseTabs by distinguishing benign disconnection/session-timeout
errors (log as warn and treat as closed) from real close failures: inspect the
error returned by browser.close(), map known benign patterns (e.g., "oneshot
canceled", websocket disconnect/session timeout) to warnings and continue, but
for other errors collect/return Err (propagate an error result) instead of
silently succeeding; update the CloseBrowser control flow around
browser.close().await and the tracing::warn call to follow the same
error-collection/return behavior used in CloseTabs.
---
Outside diff comments:
In `@src/config/types.rs`:
- Around line 840-846: The default for BrowserConfig enables JavaScript
evaluation by setting evaluate_enabled to true; revert this to keep JS
evaluation opt-in by default by changing BrowserConfig::default so
evaluate_enabled is false and ensure any existing merge/override logic that uses
DefaultsConfig and unwrap_or(base.evaluate_enabled) continues to respect
explicit per-agent settings rather than flipping to true implicitly—update the
BrowserConfig::default implementation (symbol: BrowserConfig, field:
evaluate_enabled) accordingly.
---
Nitpick comments:
In `@interface/src/components/CortexChatPanel.tsx`:
- Around line 181-189: Both CortexChatInput's adjustHeight (in
CortexChatPanel.tsx) and FloatingChatInput's adjustHeight (in WebChatPanel.tsx)
duplicate textarea resizing logic; extract that logic into a shared utility
(e.g., export function adjustTextareaHeight(textarea: HTMLTextAreaElement,
maxHeight: number) in utils/textarea.ts or a custom hook useAutoResizeTextarea)
and replace each component's adjustHeight with a thin wrapper that calls the
shared function passing the appropriate maxHeight (160 for CortexChatInput, 200
for FloatingChatInput); update imports and ensure textareaRef.current null
checks remain in the components.
In `@interface/src/components/ModelSelect.tsx`:
- Around line 126-140: The current flatList useMemo builds an array (flatList)
and render code repeatedly calls flatList.indexOf(model), causing O(n²)
rerenders; create a memoized map from model id to index (e.g., idToIndex) using
useMemo alongside flatList (dependent on sortedProviders and grouped) and
replace all flatList.indexOf(model) calls with idToIndex[model.id] (also apply
the same change where models/value are used at lines referenced around
selectedModel and the other occurrence at 314-315); ensure the map is kept in
sync by computing it from the same flatList source so lookups become O(1).
In `@interface/src/components/WebChatPanel.tsx`:
- Line 194: The property messagesEndRef is typed redundantly as
React.RefObject<HTMLDivElement | null>; update its declaration to
React.RefObject<HTMLDivElement> to rely on RefObject's built-in nullable current
type (locate the messagesEndRef declaration in the WebChatPanel component and
replace its type accordingly), ensuring any places that access
messagesEndRef.current remain type-safe and adjust any explicit null assertions
if present.
In `@scripts/bundle-sidecar.ps1`:
- Around line 20-22: The script assumes rustc exists and that Select-String
returns a match when computing $HostTriple, which can fail silently; add
defensive checks: first verify rustc is in PATH (e.g., test running "rustc -vV"
and capture non-zero/empty output) and if it fails emit a clear error and exit,
then ensure Select-String produced a match before accessing Split — if no match,
emit a descriptive error or fall back to a sensible default; finally keep the
existing $TargetTriple assignment but only use the parsed $HostTriple when it
was successfully validated. Refer to the symbols $HostTriple, $TargetTriple,
rustc and Select-String when locating where to add these validations.
- Around line 43-52: PSScriptAnalyzer flags the use of Write-Host in the
bundle-sidecar script (occurrences of Write-Host "Copied $SrcBin -> $DestBin"
and Write-Host "Sidecar binary ready."), so either replace those Write-Host
calls with Write-Information to route messages through the standard output
streams or add a PSScriptAnalyzer suppression at the top of the script to
disable PSAvoidUsingWriteHost (e.g., add a comment to disable
PSAvoidUsingWriteHost or the suggested Requires header); choose one approach and
apply it consistently for the Write-Host usages in this script.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 53a80cdc-8433-4164-ab3e-d0f5e1d01d21
⛔ Files ignored due to path filters (50)
desktop/src-tauri/gen/schemas/windows-schema.jsonis excluded by!**/gen/**,!**/*.json,!**/gen/**desktop/src-tauri/icons/64x64.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/Square107x107Logo.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/Square142x142Logo.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/Square150x150Logo.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/Square284x284Logo.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/Square30x30Logo.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/Square310x310Logo.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/Square44x44Logo.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/Square71x71Logo.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/Square89x89Logo.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/StoreLogo.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-anydpi-v26/ic_launcher.xmlis excluded by!**/*.xmldesktop/src-tauri/icons/android/mipmap-hdpi/ic_launcher.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-hdpi/ic_launcher_foreground.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-hdpi/ic_launcher_round.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-mdpi/ic_launcher.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-mdpi/ic_launcher_foreground.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-mdpi/ic_launcher_round.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-xhdpi/ic_launcher.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-xhdpi/ic_launcher_foreground.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-xhdpi/ic_launcher_round.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-xxhdpi/ic_launcher.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-xxhdpi/ic_launcher_foreground.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-xxhdpi/ic_launcher_round.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-xxxhdpi/ic_launcher.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-xxxhdpi/ic_launcher_foreground.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-xxxhdpi/ic_launcher_round.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/values/ic_launcher_background.xmlis excluded by!**/*.xmldesktop/src-tauri/icons/icon.icois excluded by!**/*.ico,!**/*.icodesktop/src-tauri/icons/icon.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-20x20@1x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-20x20@2x-1.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-20x20@2x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-20x20@3x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-29x29@1x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-29x29@2x-1.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-29x29@2x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-29x29@3x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-40x40@1x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-40x40@2x-1.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-40x40@2x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-40x40@3x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-512@2x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-60x60@2x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-60x60@3x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-76x76@1x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-76x76@2x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-83.5x83.5@2x.pngis excluded by!**/*.png,!**/*.pngdesktop/tauri.windows.conf.jsonis excluded by!**/*.json
📒 Files selected for processing (9)
interface/src/components/CortexChatPanel.tsxinterface/src/components/ModelSelect.tsxinterface/src/components/WebChatPanel.tsxscripts/bundle-sidecar.ps1src/agent/compactor.rssrc/agent/worker.rssrc/api/models.rssrc/config/types.rssrc/tools/browser.rs
| if (e.key === "ArrowUp") { | ||
| e.preventDefault(); | ||
| setHighlightIndex((prev) => | ||
| prev > 0 ? prev - 1 : flatList.length - 1, | ||
| ); | ||
| return; |
There was a problem hiding this comment.
Mirror the closed-state setup for ArrowUp.
When the input is closed, this branch only updates highlightIndex, so the first ArrowUp press appears to do nothing. Open the menu here the same way the ArrowDown path does.
Suggested fix
if (e.key === "ArrowUp") {
e.preventDefault();
+ if (!open) {
+ setOpen(true);
+ setFilter(value);
+ }
setHighlightIndex((prev) =>
prev > 0 ? prev - 1 : flatList.length - 1,
);
return;
}📝 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.
| if (e.key === "ArrowUp") { | |
| e.preventDefault(); | |
| setHighlightIndex((prev) => | |
| prev > 0 ? prev - 1 : flatList.length - 1, | |
| ); | |
| return; | |
| if (e.key === "ArrowUp") { | |
| e.preventDefault(); | |
| if (!open) { | |
| setOpen(true); | |
| setFilter(value); | |
| } | |
| setHighlightIndex((prev) => | |
| prev > 0 ? prev - 1 : flatList.length - 1, | |
| ); | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@interface/src/components/ModelSelect.tsx` around lines 209 - 214, The ArrowUp
key branch in the key handler (in ModelSelect.tsx) only updates
setHighlightIndex using flatList and doesn't open the menu when the input is
closed; mirror the ArrowDown behavior by checking the closed state and calling
the same open/menu-open path (the same function or state toggle used by the
ArrowDown branch) before updating setHighlightIndex so the first ArrowUp press
opens the menu and moves the highlight as expected. Ensure you reference the
same open/menu toggle used elsewhere in this component so behavior is consistent
with the ArrowDown branch.
| <button | ||
| key={model.id} | ||
| type="button" | ||
| data-model-item | ||
| className={`w-full text-left px-3 py-1.5 text-sm transition-colors flex items-center justify-between gap-2 ${ | ||
| isHighlighted | ||
| ? "bg-accent/20 text-ink" | ||
| : isSelected | ||
| ? "bg-app-selected/50 text-ink" | ||
| : "text-ink hover:bg-app-selected" | ||
| }`} | ||
| onMouseDown={(e) => { | ||
| e.preventDefault(); | ||
| handleSelect(model.id); | ||
| }} | ||
| onMouseEnter={() => setHighlightIndex(flatIdx)} |
There was a problem hiding this comment.
Keep the option rows keyboard-activatable.
Line 329 only calls handleSelect from onMouseDown, so a user who tabs to a model and presses Enter/Space gets a dead button. Keep onMouseDown for blur suppression, but move the actual selection to onClick.
Suggested fix
<button
key={model.id}
type="button"
data-model-item
className={`w-full text-left px-3 py-1.5 text-sm transition-colors flex items-center justify-between gap-2 ${
isHighlighted
? "bg-accent/20 text-ink"
: isSelected
? "bg-app-selected/50 text-ink"
: "text-ink hover:bg-app-selected"
}`}
- onMouseDown={(e) => {
- e.preventDefault();
- handleSelect(model.id);
- }}
+ onMouseDown={(e) => e.preventDefault()}
+ onClick={() => handleSelect(model.id)}
onMouseEnter={() => setHighlightIndex(flatIdx)}
>📝 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.
| <button | |
| key={model.id} | |
| type="button" | |
| data-model-item | |
| className={`w-full text-left px-3 py-1.5 text-sm transition-colors flex items-center justify-between gap-2 ${ | |
| isHighlighted | |
| ? "bg-accent/20 text-ink" | |
| : isSelected | |
| ? "bg-app-selected/50 text-ink" | |
| : "text-ink hover:bg-app-selected" | |
| }`} | |
| onMouseDown={(e) => { | |
| e.preventDefault(); | |
| handleSelect(model.id); | |
| }} | |
| onMouseEnter={() => setHighlightIndex(flatIdx)} | |
| <button | |
| key={model.id} | |
| type="button" | |
| data-model-item | |
| className={`w-full text-left px-3 py-1.5 text-sm transition-colors flex items-center justify-between gap-2 ${ | |
| isHighlighted | |
| ? "bg-accent/20 text-ink" | |
| : isSelected | |
| ? "bg-app-selected/50 text-ink" | |
| : "text-ink hover:bg-app-selected" | |
| }`} | |
| onMouseDown={(e) => e.preventDefault()} | |
| onClick={() => handleSelect(model.id)} | |
| onMouseEnter={() => setHighlightIndex(flatIdx)} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@interface/src/components/ModelSelect.tsx` around lines 318 - 333, The option
buttons currently call handleSelect(model.id) only from the onMouseDown handler
which prevents keyboard activation; keep the existing onMouseDown behavior to
suppress blur but remove the call to handleSelect from onMouseDown and instead
invoke handleSelect(model.id) from an onClick handler on the same button
element; update the button that uses setHighlightIndex and onMouseEnter to add
onClick={() => handleSelect(model.id)} and ensure onMouseDown only calls
e.preventDefault() to preserve blur suppression.
scripts/bundle-sidecar.ps1
Outdated
| if ($TargetTriple -ne $HostTriple) { | ||
| cargo build @CargoFlags --target $TargetTriple --manifest-path "$RepoRoot\Cargo.toml" | ||
| $SrcBin = Join-Path $RepoRoot "target\$TargetTriple\$BuildMode\spacebot.exe" | ||
| } else { | ||
| cargo build @CargoFlags --manifest-path "$RepoRoot\Cargo.toml" | ||
| $SrcBin = Join-Path $RepoRoot "target\$BuildMode\spacebot.exe" | ||
| } |
There was a problem hiding this comment.
Missing exit code check after cargo build — script continues on build failure.
$ErrorActionPreference = "Stop" only affects PowerShell cmdlets, not external executables. If cargo build fails, the script continues and will attempt to copy a non-existent binary. Add an explicit $LASTEXITCODE check after each cargo invocation.
🐛 Proposed fix to check cargo exit code
if ($TargetTriple -ne $HostTriple) {
cargo build `@CargoFlags` --target $TargetTriple --manifest-path "$RepoRoot\Cargo.toml"
+ if ($LASTEXITCODE -ne 0) {
+ Write-Error "cargo build failed with exit code $LASTEXITCODE"
+ exit $LASTEXITCODE
+ }
$SrcBin = Join-Path $RepoRoot "target\$TargetTriple\$BuildMode\spacebot.exe"
} else {
cargo build `@CargoFlags` --manifest-path "$RepoRoot\Cargo.toml"
+ if ($LASTEXITCODE -ne 0) {
+ Write-Error "cargo build failed with exit code $LASTEXITCODE"
+ exit $LASTEXITCODE
+ }
$SrcBin = Join-Path $RepoRoot "target\$BuildMode\spacebot.exe"
}📝 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.
| if ($TargetTriple -ne $HostTriple) { | |
| cargo build @CargoFlags --target $TargetTriple --manifest-path "$RepoRoot\Cargo.toml" | |
| $SrcBin = Join-Path $RepoRoot "target\$TargetTriple\$BuildMode\spacebot.exe" | |
| } else { | |
| cargo build @CargoFlags --manifest-path "$RepoRoot\Cargo.toml" | |
| $SrcBin = Join-Path $RepoRoot "target\$BuildMode\spacebot.exe" | |
| } | |
| if ($TargetTriple -ne $HostTriple) { | |
| cargo build `@CargoFlags` --target $TargetTriple --manifest-path "$RepoRoot\Cargo.toml" | |
| if ($LASTEXITCODE -ne 0) { | |
| Write-Error "cargo build failed with exit code $LASTEXITCODE" | |
| exit $LASTEXITCODE | |
| } | |
| $SrcBin = Join-Path $RepoRoot "target\$TargetTriple\$BuildMode\spacebot.exe" | |
| } else { | |
| cargo build `@CargoFlags` --manifest-path "$RepoRoot\Cargo.toml" | |
| if ($LASTEXITCODE -ne 0) { | |
| Write-Error "cargo build failed with exit code $LASTEXITCODE" | |
| exit $LASTEXITCODE | |
| } | |
| $SrcBin = Join-Path $RepoRoot "target\$BuildMode\spacebot.exe" | |
| } |
🧰 Tools
🪛 PSScriptAnalyzer (1.24.0)
[warning] Missing BOM encoding for non-ASCII encoded file 'bundle-sidecar.ps1'
(PSUseBOMForUnicodeEncodedFile)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/bundle-sidecar.ps1` around lines 35 - 41, After each external cargo
build invocation (the two branches that run "cargo build" when TargetTriple vs
HostTriple are compared), check $LASTEXITCODE immediately and abort if non-zero;
in practice, after the cargo build calls referenced by CargoFlags, TargetTriple,
HostTriple, BuildMode and RepoRoot, inspect $LASTEXITCODE and call exit
$LASTEXITCODE or throw an error before continuing to set $SrcBin or perform any
copy so the script does not proceed when the build failed.
src/agent/compactor.rs
Outdated
| let mut remove_count = total / 2; | ||
|
|
||
| // Advance past User messages containing ToolResult blocks so we never | ||
| // orphan tool_results whose matching tool_use was in a removed assistant | ||
| // message (the Anthropic API rejects orphaned tool_result blocks). | ||
| while remove_count < total.saturating_sub(2) { | ||
| let has_tool_result = matches!( | ||
| history.get(remove_count), | ||
| Some(Message::User { content }) | ||
| if content.iter().any(|item| | ||
| matches!(item, UserContent::ToolResult(_))) | ||
| ); | ||
| if has_tool_result { | ||
| remove_count += 1; | ||
| } else { | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
The boundary scan still misses the total - 2 orphan case.
At Lines 180-192 and 235-247 the scan only runs while remove_count < total.saturating_sub(2). For a 4-message history like [User, Assistant(ToolCall), User(ToolResult), Assistant], aggressive/emergency compaction sets remove_count = 2, never enters the loop, and the kept history still starts with the orphaned ToolResult. That means the Anthropic rejection this change is trying to avoid can still happen. Please handle the == total.saturating_sub(2) case here too—either by allowing one more removal or by splitting the boundary User message instead of keeping it wholesale.
Also applies to: 226-247
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/agent/compactor.rs` around lines 175 - 192, The boundary scan can miss
the case when remove_count == total.saturating_sub(2), leaving an orphaned User
ToolResult; update the loop in compactor.rs that advances remove_count (the one
using history.get(remove_count) and matching Message::User /
UserContent::ToolResult) to include the equality case (e.g., use <= or remove
the strict < bound) and, when the scan reaches exactly total.saturating_sub(2),
handle it by splitting the boundary User message instead of keeping it whole
(i.e., trim or split the User content so ToolResult is not orphaned) — apply the
same change to the analogous scan block referenced around lines 226-247.
src/agent/worker.rs
Outdated
| let mut remove_count = ((total as f32 * fraction) as usize) | ||
| .max(1) | ||
| .min(total.saturating_sub(2)); | ||
|
|
||
| // Advance the boundary past User messages that contain ToolResult blocks. | ||
| // If we stop right after an Assistant message with ToolCalls, the next | ||
| // User message holds the corresponding ToolResults. Leaving those orphaned | ||
| // causes the Anthropic API to reject the request with: | ||
| // "unexpected tool_use_id found in tool_result blocks" | ||
| while remove_count < total.saturating_sub(2) { | ||
| let has_tool_result = matches!( | ||
| history.get(remove_count), | ||
| Some(rig::message::Message::User { content }) | ||
| if content.iter().any(|item| | ||
| matches!(item, rig::message::UserContent::ToolResult(_))) | ||
| ); | ||
| if has_tool_result { | ||
| remove_count += 1; | ||
| } else { | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Overflow recovery still has the same off-by-one hole.
At Lines 758-770 remove_count is clamped to total.saturating_sub(2) before the scan, and the loop only runs while it is strictly smaller. With fraction = 0.75 and a 5-message history, remove_count becomes 3; if history[3] is the User(ToolResult) paired with a removed assistant tool call, the retry keeps the orphan and Anthropic rejects the follow-up again. This path needs the same == total - 2 handling as the channel compactor path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/agent/worker.rs` around lines 749 - 770, The overflow-recovery loop can
still leave an orphaned User ToolResult when remove_count equals
total.saturating_sub(2); change the loop bounds so the scan includes that index.
Specifically, adjust the loop condition around remove_count/total (and the
initial clamping if needed) so the while allows checking when remove_count ==
total.saturating_sub(2) (e.g., use <= instead of < or clamp differently) and
keep the existing logic that increments remove_count when
history.get(remove_count) matches rig::message::Message::User with
UserContent::ToolResult; this mirrors the channel compactor fix so the paired
ToolResult is advanced past the removal boundary.
src/api/models.rs
Outdated
| if has_key("ollama_base_url", "OLLAMA_BASE_URL") | ||
| || has_key("ollama_key", "OLLAMA_API_KEY") | ||
| { | ||
| providers.push("ollama"); | ||
| } |
There was a problem hiding this comment.
New providers are configured but never resolvable from the catalog.
Line 341-Line 369 adds ollama, nvidia, and github-copilot to configured providers, but direct_provider_mapping (Line 104-Line 123) never emits these provider IDs. As a result, catalog models for these providers fall back to "openrouter" and won’t match configured.contains(...) or provider=<name> filters.
Proposed fix
fn direct_provider_mapping(models_dev_id: &str) -> Option<&'static str> {
match models_dev_id {
"anthropic" => Some("anthropic"),
"openai" => Some("openai"),
+ "ollama" => Some("ollama"),
+ "nvidia" => Some("nvidia"),
+ "github-copilot" => Some("github-copilot"),
"kilo" => Some("kilo"),
"deepseek" => Some("deepseek"),
"xai" => Some("xai"),
"mistral" => Some("mistral"),
"gemini" | "google" => Some("gemini"),Also applies to: 352-354, 367-369
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/models.rs` around lines 341 - 345, The new branches that push
"ollama", "nvidia", and "github-copilot" into providers are never translated by
direct_provider_mapping, so catalog lookups fall back to "openrouter" and
filters like configured.contains(...) or provider=<name> fail; update
direct_provider_mapping to emit the corresponding provider IDs for Ollama,
NVIDIA, and GitHub Copilot (or add a mapping helper used by
direct_provider_mapping) so that provider strings produced by
providers.push("ollama"/"nvidia"/"github-copilot") match catalog entries and the
configured.contains(...) / provider=<name> filters. Ensure the mapping is
symmetric with has_key(...) usage and update any tests or usages of
direct_provider_mapping to include the new IDs.
src/tools/browser.rs
Outdated
| if let Some(mut browser) = browser { | ||
| if let Err(error) = browser.close().await { | ||
| // The browser connection may already be dead (e.g. "oneshot canceled" | ||
| // from a dropped WebSocket). That's fine — the browser is effectively | ||
| // closed. Log it but don't fail the tool call. | ||
| tracing::warn!( | ||
| policy = "close_browser", | ||
| %error, | ||
| "browser.close() failed, treating as already closed" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In chromiumoxide, what documented/observed errors can Browser::close().await return, and which ones indicate an already-closed/disconnected session vs a real close failure?
💡 Result:
Browser::close.await returns Result<, CdpError>. Documented/observed errors from official docs and issues include: - CdpError::Ws(tungstenite::Error): WebSocket errors, e.g., connection reset or closed (Io(Os { code: 10054, kind: ConnectionReset, message: "An existing connection was forcibly closed by the remote host." })). These typically indicate an already-closed or disconnected session, as the browser process may have terminated or WS disconnected before/during close. - CdpError::Serde(serde_json::Error): JSON deserialization failures on CDP responses. Observed when Chrome sends unexpected messages; library now skips them but could error on close command response. - CdpError::Chrome(chromiumoxide_types::Error): Chrome DevTools Protocol errors from the Browser.close command. - CdpError::ChromeMessage(String): Non-standard CDP error messages. - Other potential: CdpError::Io, CdpError::NoResponse, CdpError::Timeout. Already-closed/disconnected vs real failure: - WS/Io errors (e.g., ConnectionReset) indicate the session was already closed/disconnected (browser exited independently). Safe to treat as success. - Serde/Chrome errors indicate real failures in command execution/response (e.g., browser rejected close, invalid state). - Docs recommend calling close then wait for spawned browsers to avoid zombies; close failure doesn't prevent Drop from cleaning up. Examples from issues: WS Connection error on handler (not directly close, but related to session end); Serde on invalid messages during operation.
Citations:
- 1: https://docs.rs/chromiumoxide/latest/chromiumoxide/browser/struct.Browser.html
- 2: https://docs.rs/spider_chrome/latest/chromiumoxide/error/enum.CdpError.html
- 3: Random crashes when not using headless mode mattsse/chromiumoxide#210
- 4: bug: Chromiumoxide stopped working with the latest version of Chrome mattsse/chromiumoxide#243
- 5: https://github.com/mattsse/chromiumoxide/blob/main/src/browser.rs
- 6: https://github.com/caido/dependency-chromiumoxide
🏁 Script executed:
# First, let's look at the full context around the flagged lines
sed -n '2050,2085p' src/tools/browser.rsRepository: spacedriveapp/spacebot
Length of output: 1606
🏁 Script executed:
# Check the function signature to understand the return type
sed -n '2000,2070p' src/tools/browser.rs | grep -A 50 "fn\|return\|Result"Repository: spacedriveapp/spacebot
Length of output: 1968
🏁 Script executed:
# Search for how CdpError is used elsewhere in the file
rg "CdpError" src/tools/browser.rs -A 3 -B 1Repository: spacedriveapp/spacebot
Length of output: 48
🏁 Script executed:
# Check what chromiumoxide error types are available
rg "use.*chromiumoxide" src/tools/browser.rsRepository: spacedriveapp/spacebot
Length of output: 543
🏁 Script executed:
# Get the function signature to confirm return type
grep -B 20 "ClosePolicy::CloseBrowser =>" src/tools/browser.rs | grep -E "fn |async|Result"Repository: spacedriveapp/spacebot
Length of output: 48
🏁 Script executed:
# Check what error type browser.close() returns - look for type annotations
sed -n '1900,2100p' src/tools/browser.rs | grep -A 5 "fn.*close\|Result\|BrowserError"Repository: spacedriveapp/spacebot
Length of output: 1729
🏁 Script executed:
# Look at the function definition for the close method to understand its return type
sed -n '1950,2000p' src/tools/browser.rsRepository: spacedriveapp/spacebot
Length of output: 1641
🏁 Script executed:
# Search for BrowserError definition and how it's constructed
rg "struct BrowserError|impl BrowserError" src/tools/browser.rs -A 5Repository: spacedriveapp/spacebot
Length of output: 256
🏁 Script executed:
# Check if CdpError is imported or used
rg "CdpError\|from_cdp\|.into()" src/tools/browser.rs | head -20Repository: spacedriveapp/spacebot
Length of output: 48
🏁 Script executed:
# Find the actual close() method signature in chromiumoxide
# First check what Browser type is and look for close implementation
rg "impl.*Browser" src/tools/browser.rs -A 10 | head -30Repository: spacedriveapp/spacebot
Length of output: 1115
🏁 Script executed:
# Look for the browser variable type annotation
sed -n '2045,2070p' src/tools/browser.rs | grep -B 20 "browser.close"Repository: spacedriveapp/spacebot
Length of output: 1135
🏁 Script executed:
# Check the state struct to see what type browser is
rg "struct.*State|browser:" src/tools/browser.rs -A 5 | head -40Repository: spacedriveapp/spacebot
Length of output: 1521
🏁 Script executed:
# Look at chromiumoxide documentation - check if CdpError variants are public/matchable
python3 << 'EOF'
# Let's check what information we can gather about CdpError from the codebase
import subprocess
result = subprocess.run(['rg', 'CdpError', 'src/tools/browser.rs'], capture_output=True, text=True)
print("CdpError references in browser.rs:")
print(result.stdout)
EOFRepository: spacedriveapp/spacebot
Length of output: 101
🏁 Script executed:
# Check if there are any error conversion patterns or match statements
rg "match.*error|Err\(|if let Err" src/tools/browser.rs | grep -v "//" | head -15Repository: spacedriveapp/spacebot
Length of output: 895
🏁 Script executed:
# Check what error type browser.close() returns - look at Browser impl
grep -n "close()" src/tools/browser.rsRepository: spacedriveapp/spacebot
Length of output: 312
🏁 Script executed:
# Let's look at how other chromiumoxide errors are handled - check page.close()
sed -n '2003,2015p' src/tools/browser.rsRepository: spacedriveapp/spacebot
Length of output: 613
🏁 Script executed:
# Check if chromiumoxide_types or chromiumoxide::error is imported
rg "use chromiumoxide" src/tools/browser.rs | grep -i errorRepository: spacedriveapp/spacebot
Length of output: 48
🏁 Script executed:
# Try to determine if CdpError can be imported and pattern matched
# Check chromiumoxide imports and see what's available
head -50 src/tools/browser.rsRepository: spacedriveapp/spacebot
Length of output: 2190
🏁 Script executed:
# Search for any error handling with pattern matching to see the pattern used in this file
rg "match.*error|CdpError::" src/tools/browser.rsRepository: spacedriveapp/spacebot
Length of output: 48
🏁 Script executed:
# Check if chromiumoxide::error or CdpError can be imported
fd "Cargo.toml" | head -5 | xargs grep -l "chromiumoxide"Repository: spacedriveapp/spacebot
Length of output: 76
🏁 Script executed:
# Look at Cargo.toml to see chromiumoxide version and features
cat Cargo.toml | grep -A 5 "chromiumoxide"Repository: spacedriveapp/spacebot
Length of output: 309
🏁 Script executed:
# Check if CdpError type is used anywhere or if error handling uses Display/to_string
rg "error\.to_string\|Display\|fmt" src/tools/browser.rs | head -10Repository: spacedriveapp/spacebot
Length of output: 48
🏁 Script executed:
# Let's see how the error is being displayed in tracing::warn
# Check what traits are available on the error
sed -n '2058,2068p' src/tools/browser.rsRepository: spacedriveapp/spacebot
Length of output: 669
🏁 Script executed:
# See if there are other places where chromiumoxide errors are handled with pattern matching
fd "\.rs" src/ | xargs grep -l "chromiumoxide" | xargs grep -l "match.*error\|CdpError" 2>/dev/null | head -5Repository: spacedriveapp/spacebot
Length of output: 48
🏁 Script executed:
# Check chromiumoxide 0.8 docs - see if CdpError is public and has matchable variants
# Let's search for any existing code that does pattern match on CdpError
rg "CdpError::" . --type rs 2>/dev/null | head -10Repository: spacedriveapp/spacebot
Length of output: 48
🏁 Script executed:
# Try to understand if the error type can be inspected - check what methods are available
# Look at how similar errors are handled elsewhere in the codebase
rg "\.await\s*\)" src/tools/browser.rs -A 2 | grep -A 2 "Err" | head -20Repository: spacedriveapp/spacebot
Length of output: 48
🏁 Script executed:
# Let's check the actual drop implementation and understand the cleanup behavior
sed -n '1992,2020p' src/tools/browser.rsRepository: spacedriveapp/spacebot
Length of output: 1287
🏁 Script executed:
# Look at what happens in Drop for BrowserState
rg -A 15 "impl Drop for BrowserState" src/tools/browser.rsRepository: spacedriveapp/spacebot
Length of output: 769
🏁 Script executed:
# Check if there's documentation or comments about error suppression patterns elsewhere
rg "safe to ignore|already|cleanup|drop" src/tools/browser.rs | head -10Repository: spacedriveapp/spacebot
Length of output: 722
🏁 Script executed:
# Let's check if chromiumoxide 0.8 has a public CdpError enum that can be pattern matched
# Try to find any imports or usage of error types
rg "use.*error|CdpError|Error" src/tools/browser.rs | grep "use\|::" | head -20Repository: spacedriveapp/spacebot
Length of output: 1438
🏁 Script executed:
# Check if there's any conditional logic on error types in chromiumoxide or similar crates
# Look at the entire error handling flow for CloseTabs to see if it handles errors differently
sed -n '2010,2045p' src/tools/browser.rsRepository: spacedriveapp/spacebot
Length of output: 1570
🏁 Script executed:
# Check what happens after close_errors are collected in CloseTabs
sed -n '2025,2055p' src/tools/browser.rsRepository: spacedriveapp/spacebot
Length of output: 1419
🏁 Script executed:
# Check the full function return path for CloseBrowser to be sure
sed -n '2040,2090p' src/tools/browser.rsRepository: spacedriveapp/spacebot
Length of output: 2239
🏁 Script executed:
# Verify the inconsistency: check what CloseTabs does with close errors
sed -n '2019,2037p' src/tools/browser.rsRepository: spacedriveapp/spacebot
Length of output: 776
🏁 Script executed:
# Check if CdpError can be imported from chromiumoxide and what variants it has
# This will help validate if pattern matching is possible vs string matching
python3 << 'EOF'
import subprocess
import json
# Try to check chromiumoxide documentation or source
result = subprocess.run(
['rg', 'chromiumoxide.*error|CdpError', 'Cargo.lock'],
capture_output=True,
text=True
)
print("Cargo.lock chromiumoxide references:")
print(result.stdout[:500] if result.stdout else "Not found")
# Check if there's a Cargo.lock to understand exact version
result2 = subprocess.run(['cat', 'Cargo.lock'], capture_output=True, text=True)
lines = result2.stdout.split('\n')
for i, line in enumerate(lines):
if 'chromiumoxide' in line.lower() and i < len(lines) - 5:
print('\n'.join(lines[i:min(i+10, len(lines))]))
break
EOFRepository: spacedriveapp/spacebot
Length of output: 412
Inconsistency: CloseBrowser silently suppresses close failures while CloseTabs propagates them.
Lines 2058-2068 suppress all browser.close() errors and return success; this contradicts CloseTabs behavior (lines 2019-2037), which collects close errors and returns Err if any occurred. Silent suppression can mask real shutdown failures and return false "Browser closed" success.
Known benign errors (WebSocket disconnection, session timeouts) exist, but suppressing all errors without distinction is unreliable. Align with CloseTabs error handling: distinguish between benign disconnection errors and real close failures.
🔧 Suggested fix
if let Some(mut browser) = browser {
if let Err(error) = browser.close().await {
- // The browser connection may already be dead (e.g. "oneshot canceled"
- // from a dropped WebSocket). That's fine — the browser is effectively
- // closed. Log it but don't fail the tool call.
- tracing::warn!(
- policy = "close_browser",
- %error,
- "browser.close() failed, treating as already closed"
- );
+ let error_text = error.to_string();
+ let benign_disconnect = error_text.contains("oneshot canceled")
+ || error_text.contains("channel closed")
+ || error_text.contains("connection reset");
+ if benign_disconnect {
+ tracing::warn!(
+ policy = "close_browser",
+ %error,
+ "browser.close() failed due to disconnected session; treating as already closed"
+ );
+ } else {
+ return Err(BrowserError::new(format!(
+ "failed to close browser: {error}"
+ )));
+ }
}
}📝 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.
| if let Some(mut browser) = browser { | |
| if let Err(error) = browser.close().await { | |
| // The browser connection may already be dead (e.g. "oneshot canceled" | |
| // from a dropped WebSocket). That's fine — the browser is effectively | |
| // closed. Log it but don't fail the tool call. | |
| tracing::warn!( | |
| policy = "close_browser", | |
| %error, | |
| "browser.close() failed, treating as already closed" | |
| ); | |
| } | |
| if let Some(mut browser) = browser { | |
| if let Err(error) = browser.close().await { | |
| let error_text = error.to_string(); | |
| let benign_disconnect = error_text.contains("oneshot canceled") | |
| || error_text.contains("channel closed") | |
| || error_text.contains("connection reset"); | |
| if benign_disconnect { | |
| tracing::warn!( | |
| policy = "close_browser", | |
| %error, | |
| "browser.close() failed due to disconnected session; treating as already closed" | |
| ); | |
| } else { | |
| return Err(BrowserError::new(format!( | |
| "failed to close browser: {error}" | |
| ))); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tools/browser.rs` around lines 2057 - 2067, CloseBrowser currently
swallows all errors from browser.close() while CloseTabs collects and returns
errors; change CloseBrowser to mirror CloseTabs by distinguishing benign
disconnection/session-timeout errors (log as warn and treat as closed) from real
close failures: inspect the error returned by browser.close(), map known benign
patterns (e.g., "oneshot canceled", websocket disconnect/session timeout) to
warnings and continue, but for other errors collect/return Err (propagate an
error result) instead of silently succeeding; update the CloseBrowser control
flow around browser.close().await and the tracing::warn call to follow the same
error-collection/return behavior used in CloseTabs.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
interface/src/components/WebChatPanel.tsx (1)
185-227: Keep the live footer separate from the historical timeline.
timeline.map(...)is the expensive work here, butisTypinganderrorare bundled into the same memoized component, so every typing-start/typing-stop or error change still remaps the full history. Pulling the footer out—or memoizing rows byitem.id—would preserve more of the perf win on long sessions.Worth confirming in React Profiler that toggling
isTypingno longer rerenders the historical bubbles after the split.Possible refactor
-const MessageList = memo(function MessageList({ - timeline, - isTyping, - error, -}: { - timeline: TimelineItem[]; - isTyping: boolean; - error: string | null; -}) { +const MessageHistory = memo(function MessageHistory({ + timeline, +}: { + timeline: TimelineItem[]; +}) { return ( <> {timeline.map((item) => { @@ - {/* Typing indicator */} - {isTyping && <ThinkingIndicator />} - - {error && ( - <div className="rounded-lg border border-red-500/20 bg-red-500/5 px-4 py-3 text-sm text-red-400"> - {error} - </div> - )} </> ); }); @@ - <MessageList - timeline={timeline} - isTyping={isTyping} - error={error} - /> + <MessageHistory timeline={timeline} /> + {isTyping && <ThinkingIndicator />} + {error && ( + <div className="rounded-lg border border-red-500/20 bg-red-500/5 px-4 py-3 text-sm text-red-400"> + {error} + </div> + )}Also applies to: 277-281
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/components/WebChatPanel.tsx` around lines 185 - 227, The MessageList currently re-maps the entire timeline whenever isTyping or error changes; fix this by extracting the live footer (the {isTyping && <ThinkingIndicator />} and the {error && <div ...>{error}</div>} block) into a separate component (e.g., LiveFooter) rendered outside the timeline.map, or alternatively create a memoized MessageRow component (e.g., function MessageRow({item}) wrapped with memo and keyed by item.id) and replace the inline JSX inside timeline.map with <MessageRow item={item} /> so timeline items don't re-render when isTyping/error change; update MessageList to render timeline.map of memoized rows plus the LiveFooter and verify in React Profiler that toggling isTyping no longer re-renders historical message bubbles.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@interface/src/components/CortexChatPanel.tsx`:
- Around line 191-199: doSubmit currently clears the textarea and draft
immediately, which can drop the first message when the send is rejected while
threadId is null; change doSubmit (and the other similar handlers at the noted
locations) to either (A) gate submission by checking threadId/newThread
readiness before calling onSubmit, or (B) make onSubmit return a boolean/Promise
that indicates success and only clear textareaRef.current.value, call
setHasText(false), and adjustHeight() after a successful send; update callers of
onSubmit as needed and reference doSubmit, textareaRef, onSubmit, threadId, and
the useCortexChat hook to locate and coordinate the change.
- Around line 373-376: The CortexChatPanel rendering collapses newlines because
the message bubble uses a <p> that renders message.content without preserving
whitespace; update the bubble that renders message.content in the
CortexChatPanel component to use the same whitespace handling as WebChatPanel
(e.g., render message.content inside an element/class that applies CSS
whitespace: pre-wrap / Tailwind "whitespace-pre-wrap") so user multi-line
prompts (Shift+Enter) appear with line breaks preserved. Ensure you change the
element rendering message.content (the <p className="text-sm text-ink">...) to
the whitespace-preserving variant and keep existing styling intact.
---
Nitpick comments:
In `@interface/src/components/WebChatPanel.tsx`:
- Around line 185-227: The MessageList currently re-maps the entire timeline
whenever isTyping or error changes; fix this by extracting the live footer (the
{isTyping && <ThinkingIndicator />} and the {error && <div ...>{error}</div>}
block) into a separate component (e.g., LiveFooter) rendered outside the
timeline.map, or alternatively create a memoized MessageRow component (e.g.,
function MessageRow({item}) wrapped with memo and keyed by item.id) and replace
the inline JSX inside timeline.map with <MessageRow item={item} /> so timeline
items don't re-render when isTyping/error change; update MessageList to render
timeline.map of memoized rows plus the LiveFooter and verify in React Profiler
that toggling isTyping no longer re-renders historical message bubbles.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ccb4a198-19ca-4045-83ec-813ead6ce16b
📒 Files selected for processing (2)
interface/src/components/CortexChatPanel.tsxinterface/src/components/WebChatPanel.tsx
| const doSubmit = () => { | ||
| const textarea = textareaRef.current; | ||
| if (!textarea) return; | ||
| const trimmed = textarea.value.trim(); | ||
| if (!trimmed) return; | ||
| textarea.value = ""; | ||
| setHasText(false); | ||
| adjustHeight(); | ||
| onSubmit(trimmed); |
There was a problem hiding this comment.
Only clear the Cortex draft after the send is accepted.
interface/src/hooks/useCortexChat.ts:81-103 still no-ops while threadId is null, but doSubmit() clears the textarea before this callback can reject. That makes the first message easy to lose during fresh-thread startup or right after newThread(). Please gate the input on threadId, or have onSubmit report whether the send was accepted and only clear on success.
Possible fix
const CortexChatInput = memo(function CortexChatInput({
onSubmit,
isStreaming,
+ canSubmit,
}: {
- onSubmit: (text: string) => void;
+ onSubmit: (text: string) => boolean;
isStreaming: boolean;
+ canSubmit: boolean;
}) {
@@
const doSubmit = () => {
const textarea = textareaRef.current;
if (!textarea) return;
const trimmed = textarea.value.trim();
if (!trimmed) return;
- textarea.value = "";
- setHasText(false);
- adjustHeight();
- onSubmit(trimmed);
+ if (!onSubmit(trimmed)) return;
+ textarea.value = "";
+ setHasText(false);
+ adjustHeight();
};
@@
- disabled={isStreaming}
+ disabled={isStreaming || !canSubmit}
@@
- disabled={isStreaming || !hasText}
+ disabled={isStreaming || !hasText || !canSubmit} const handleSubmit = useCallback(
(text: string) => {
- if (isStreaming) return;
+ if (isStreaming || !threadId) return false;
sendMessage(text);
+ return true;
},
- [isStreaming, sendMessage],
+ [isStreaming, threadId, sendMessage],
);
@@
<CortexChatInput
onSubmit={handleSubmit}
isStreaming={isStreaming}
+ canSubmit={!!threadId}
/>Also applies to: 439-445, 557-560
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@interface/src/components/CortexChatPanel.tsx` around lines 191 - 199,
doSubmit currently clears the textarea and draft immediately, which can drop the
first message when the send is rejected while threadId is null; change doSubmit
(and the other similar handlers at the noted locations) to either (A) gate
submission by checking threadId/newThread readiness before calling onSubmit, or
(B) make onSubmit return a boolean/Promise that indicates success and only clear
textareaRef.current.value, call setHasText(false), and adjustHeight() after a
successful send; update callers of onSubmit as needed and reference doSubmit,
textareaRef, onSubmit, threadId, and the useCortexChat hook to locate and
coordinate the change.
| <div className="flex justify-end"> | ||
| <div className="max-w-[85%] rounded-2xl rounded-br-md bg-app-hover/30 px-3 py-2"> | ||
| <p className="text-sm text-ink">{message.content}</p> | ||
| </div> |
There was a problem hiding this comment.
Preserve multiline user prompts in the Cortex bubble.
The new textarea supports Shift+Enter, but this <p> still collapses \n into spaces, so multi-line prompts render as a single line after send. Matching the whitespace handling already used in interface/src/components/WebChatPanel.tsx:202-205 would keep the displayed message faithful to what the user entered.
Possible fix
- <p className="text-sm text-ink">{message.content}</p>
+ <p className="break-words whitespace-pre-wrap text-sm text-ink">
+ {message.content}
+ </p>📝 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.
| <div className="flex justify-end"> | |
| <div className="max-w-[85%] rounded-2xl rounded-br-md bg-app-hover/30 px-3 py-2"> | |
| <p className="text-sm text-ink">{message.content}</p> | |
| </div> | |
| <div className="flex justify-end"> | |
| <div className="max-w-[85%] rounded-2xl rounded-br-md bg-app-hover/30 px-3 py-2"> | |
| <p className="break-words whitespace-pre-wrap text-sm text-ink"> | |
| {message.content} | |
| </p> | |
| </div> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@interface/src/components/CortexChatPanel.tsx` around lines 373 - 376, The
CortexChatPanel rendering collapses newlines because the message bubble uses a
<p> that renders message.content without preserving whitespace; update the
bubble that renders message.content in the CortexChatPanel component to use the
same whitespace handling as WebChatPanel (e.g., render message.content inside an
element/class that applies CSS whitespace: pre-wrap / Tailwind
"whitespace-pre-wrap") so user multi-line prompts (Shift+Enter) appear with line
breaks preserved. Ensure you change the element rendering message.content (the
<p className="text-sm text-ink">...) to the whitespace-preserving variant and
keep existing styling intact.
1 participant
Summary
FloatingChatInputandCortexChatInputfrom controlled (useState) to uncontrolled (ref-based) textareas so keystrokes bypass React's render cycle entirelyReact.memoto prevent re-renders when parent state changes from SSE eventsMessageListandCortexMessageListintoReact.memocomponents so long chat histories don't re-render on every SSE eventProblem
When chatting with an agent over a long conversation, typing in the input box was severely laggy — letters would take seconds to appear, and backspace was equally slow. This was caused by the shared SSE event bus firing frequent state updates (worker lifecycle, tool calls, streaming tokens), which triggered React re-renders of the entire chat component tree including the controlled textarea input. With hundreds of messages to reconcile, the browser couldn't process keystrokes in time.
Test plan
🤖 Generated with Claude Code