Fix build scripts and dependencies#2
Merged
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughReplaces hardcoded port 3333 with dynamic OS-assigned ports: Node backend listens on PORT=0 and prints a [BACKEND_PORT] marker; the Rust BackendManager captures stdout, parses and stores the port, and exposes it via a new Tauri command. Frontend, sidecar, scripts, tests, and docs updated to discover/use the dynamic port; socket.io deps removed and MCP config updated. Changes
Sequence Diagram(s)sequenceDiagram
participant Frontend as Frontend (Vite)
participant Tauri as Tauri IPC
participant BackendMgr as BackendManager (Rust)
participant Backend as Backend (Node)
rect rgb(235,245,255)
note right of Backend: Startup / Port discovery
Tauri->>BackendMgr: spawn backend (PORT=0)
BackendMgr->>Backend: start process (stdout piped)
Backend->>Backend: bind OS-assigned port
Backend->>BackendMgr: "[BACKEND_PORT]54321"
BackendMgr->>BackendMgr: parse & store port (Arc<Mutex<Option<u16>>>)
end
rect rgb(230,255,235)
note right of Frontend: Runtime resolution
Frontend->>Tauri: invoke("get_backend_port")
Tauri->>BackendMgr: get_port()
BackendMgr-->>Tauri: 54321
Tauri-->>Frontend: 54321
Frontend->>Frontend: cache port → getBaseURL()
end
rect rgb(255,250,230)
note right of Frontend: API traffic
Frontend->>Backend: GET /api/...
Backend-->>Frontend: 200 OK
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Changes span Rust/Node/TS, IPC surface, threading/state safety (Arc/Mutex), constructor/signature updates, many frontend call sites, scripts, tests and docs. Backend stdout parsing, concurrency correctness, and async runtime resolution require careful verification; many frontend edits are repetitive but need end-to-end validation. Poem
Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional Comments (2)
-
test-end-to-end.sh, line 123 (link)logic: Socket variable might be undefined if sidecar response doesn't contain socketPath field
-
src/hooks/useDiffStats.ts, line 52-60 (link)style: forEach with async callback may cause race conditions - consider using Promise.all for parallel execution or regular for loop for sequential execution
31 files reviewed, 21 comments
Comment on lines
+58
to
+64
| const sessionRes = await fetch(`${await getBaseURL()}/sessions/${sessionId}`); | ||
| const sessionData = await sessionRes.json(); | ||
| setSessionStatus(sessionData.status || 'idle'); | ||
| setIsCompacting(sessionData.is_compacting === 1); | ||
|
|
||
| // Load messages | ||
| const res = await fetch(`${API_BASE}/sessions/${sessionId}/messages`); | ||
| const res = await fetch(`${await getBaseURL()}/sessions/${sessionId}/messages`); |
There was a problem hiding this comment.
style: Multiple await calls in quick succession might cause performance issues. Consider batching these requests or using Promise.all if the requests are independent.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/hooks/useMessages.ts
Line: 58:64
Comment:
**style:** Multiple await calls in quick succession might cause performance issues. Consider batching these requests or using Promise.all if the requests are independent.
How can I resolve this? If you propose a fix, please make it concise.| */ | ||
| private async request<T>(endpoint: string, options: RequestInit): Promise<T> { | ||
| const url = `${this.baseURL}${endpoint}`; | ||
| const baseURL = await getBaseURL(); |
There was a problem hiding this comment.
style: This async call on every request could impact performance. Consider caching the baseURL with periodic refresh or using a singleton pattern if the port doesn't change frequently during runtime.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/services/api.ts
Line: 58:58
Comment:
**style:** This async call on every request could impact performance. Consider caching the baseURL with periodic refresh or using a singleton pattern if the port doesn't change frequently during runtime.
How can I resolve this? If you propose a fix, please make it concise.| async function loadSettings() { | ||
| try { | ||
| const response = await fetch(`${API_BASE}/api/settings`); | ||
| const response = await fetch(`${await getBaseURL()}/api/settings`); |
There was a problem hiding this comment.
style: Multiple await calls in sequence could be optimized with Promise.all for better performance
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Settings.tsx
Line: 46:46
Comment:
**style:** Multiple await calls in sequence could be optimized with Promise.all for better performance
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
59
to
76
| const mcpResponse = await fetch(`${await getBaseURL()}/api/config/mcp-servers`); | ||
| const mcpData = await mcpResponse.json(); | ||
| setMcpServers(mcpData); | ||
|
|
||
| // Load commands | ||
| const commandsResponse = await fetch(`${API_BASE}/api/config/commands`); | ||
| const commandsResponse = await fetch(`${await getBaseURL()}/api/config/commands`); | ||
| const commandsData = await commandsResponse.json(); | ||
| setCommands(commandsData); | ||
|
|
||
| // Load agents | ||
| const agentsResponse = await fetch(`${API_BASE}/api/config/agents`); | ||
| const agentsResponse = await fetch(`${await getBaseURL()}/api/config/agents`); | ||
| const agentsData = await agentsResponse.json(); | ||
| setAgents(agentsData); | ||
|
|
||
| // Load hooks | ||
| const hooksResponse = await fetch(`${API_BASE}/api/config/hooks`); | ||
| const hooksResponse = await fetch(`${await getBaseURL()}/api/config/hooks`); | ||
| const hooksData = await hooksResponse.json(); | ||
| setHooks(hooksData); |
There was a problem hiding this comment.
style: Consider batching these config API calls with Promise.all to reduce loading time
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Settings.tsx
Line: 59:76
Comment:
**style:** Consider batching these config API calls with Promise.all to reduce loading time
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| # Try to detect from backend logs | ||
| if [ -f "/tmp/backend.log" ]; then | ||
| BACKEND_PORT=$(grep '\[BACKEND_PORT\]' /tmp/backend.log | tail -1 | sed 's/.*\[BACKEND_PORT\]//') |
There was a problem hiding this comment.
style: Log parsing might fail if the log format changes or multiple backends are running - consider more robust parsing
Prompt To Fix With AI
This is a comment left during a code review.
Path: test-end-to-end.sh
Line: 17:17
Comment:
**style:** Log parsing might fail if the log format changes or multiple backends are running - consider more robust parsing
How can I resolve this? If you propose a fix, please make it concise.| ``` | ||
| ┌─────────────────┐ HTTP Requests ┌──────────────────┐ | ||
| │ Frontend │ ───────────────────────> │ Backend │ | ||
| │ (React+Vite) │ localhost:3333/api │ (Node+Express) │ |
There was a problem hiding this comment.
logic: Architecture diagram shows hardcoded localhost:3333/api which contradicts the dynamic port implementation in this PR.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/ISSUE_REPORT.md
Line: 92:92
Comment:
**logic:** Architecture diagram shows hardcoded localhost:3333/api which contradicts the dynamic port implementation in this PR.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+26
to
+75
| (async () => { | ||
| const baseURL = await getBaseURL(); | ||
|
|
||
| // Load PR status | ||
| fetch(`${API_BASE}/workspaces/${workspaceId}/pr-status`) | ||
| .then(res => res.json()) | ||
| .then(data => { | ||
| setPrStatus(data); | ||
| }) | ||
| .catch(err => { | ||
| console.error('Failed to load PR status:', err); | ||
| setPrStatus(null); | ||
| }); | ||
| // Note: Diff stats loading is handled by useDashboardData hook | ||
|
|
||
| // Load dev servers | ||
| fetch(`${API_BASE}/workspaces/${workspaceId}/dev-servers`) | ||
| .then(res => res.json()) | ||
| .then(data => { | ||
| setDevServers(data.servers || []); | ||
| }) | ||
| .catch(err => { | ||
| console.error('Failed to load dev servers:', err); | ||
| setDevServers([]); | ||
| }); | ||
| // Load PR status | ||
| fetch(`${baseURL}/workspaces/${workspaceId}/pr-status`) | ||
| .then(res => res.json()) | ||
| .then(data => { | ||
| setPrStatus(data); | ||
| }) | ||
| .catch(err => { | ||
| console.error('Failed to load PR status:', err); | ||
| setPrStatus(null); | ||
| }); | ||
|
|
||
| // Check cache first for file changes | ||
| if (fileChangesCache.current[workspaceId]) { | ||
| console.log('✅ Using cached file changes for workspace:', workspaceId); | ||
| setFileChanges(fileChangesCache.current[workspaceId]); | ||
| return; | ||
| } | ||
| // Load dev servers | ||
| fetch(`${baseURL}/workspaces/${workspaceId}/dev-servers`) | ||
| .then(res => res.json()) | ||
| .then(data => { | ||
| setDevServers(data.servers || []); | ||
| }) | ||
| .catch(err => { | ||
| console.error('Failed to load dev servers:', err); | ||
| setDevServers([]); | ||
| }); | ||
|
|
||
| // Check cache first for file changes | ||
| if (fileChangesCache.current[workspaceId]) { | ||
| console.log('✅ Using cached file changes for workspace:', workspaceId); | ||
| setFileChanges(fileChangesCache.current[workspaceId]); | ||
| return; | ||
| } | ||
|
|
||
| // Load from API if not in cache | ||
| console.log('🔄 Loading file changes for workspace:', workspaceId); | ||
| fetch(`${API_BASE}/workspaces/${workspaceId}/diff-files`) | ||
| .then(res => res.json()) | ||
| .then(data => { | ||
| const files = data.files || []; | ||
| console.log('✅ File changes loaded:', files.length, 'files'); | ||
| setFileChanges(files); | ||
| // Cache the result | ||
| fileChangesCache.current[workspaceId] = files; | ||
| }) | ||
| .catch(err => { | ||
| console.error('❌ Failed to load file changes:', err); | ||
| setFileChanges([]); | ||
| }); | ||
| // Load from API if not in cache | ||
| console.log('🔄 Loading file changes for workspace:', workspaceId); | ||
| fetch(`${baseURL}/workspaces/${workspaceId}/diff-files`) | ||
| .then(res => res.json()) | ||
| .then(data => { | ||
| const files = data.files || []; | ||
| console.log('✅ File changes loaded:', files.length, 'files'); | ||
| setFileChanges(files); | ||
| // Cache the result | ||
| fileChangesCache.current[workspaceId] = files; | ||
| }) | ||
| .catch(err => { | ||
| console.error('❌ Failed to load file changes:', err); | ||
| setFileChanges([]); | ||
| }); | ||
| })(); |
There was a problem hiding this comment.
logic: Race condition risk: async operations continue after component unmount or workspaceId change. Consider using AbortController or cleanup function to cancel in-flight requests.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/hooks/useFileChanges.ts
Line: 26:75
Comment:
**logic:** Race condition risk: async operations continue after component unmount or workspaceId change. Consider using AbortController or cleanup function to cancel in-flight requests.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+54
to
+58
| if (fileChangesCache.current[workspaceId]) { | ||
| console.log('✅ Using cached file changes for workspace:', workspaceId); | ||
| setFileChanges(fileChangesCache.current[workspaceId]); | ||
| return; | ||
| } |
There was a problem hiding this comment.
logic: Cache check happens after async getBaseURL() call but uses synchronous workspaceId - this could cause issues if workspaceId changes during the async operation.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/hooks/useFileChanges.ts
Line: 54:58
Comment:
**logic:** Cache check happens after async getBaseURL() call but uses synchronous workspaceId - this could cause issues if workspaceId changes during the async operation.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+82
to
+86
| while start.elapsed() < std::time::Duration::from_secs(5) { | ||
| if self.port.lock().unwrap().is_some() { | ||
| break; | ||
| } | ||
| std::thread::sleep(std::time::Duration::from_millis(100)); |
There was a problem hiding this comment.
style: Polling with 100ms intervals for 5 seconds could block the main thread - consider if this blocking behavior is acceptable during backend startup
Prompt To Fix With AI
This is a comment left during a code review.
Path: src-tauri/src/backend.rs
Line: 82:86
Comment:
**style:** Polling with 100ms intervals for 5 seconds could block the main thread - consider if this blocking behavior is acceptable during backend startup
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+164
to
+172
| console.log('[BACKEND_PORT]54321'); | ||
| console.log('Mock backend started'); | ||
|
|
||
| // Keep the process alive for a moment | ||
| setTimeout(() => { | ||
| console.log('Mock backend shutting down'); | ||
| process.exit(0); | ||
| }, 2000); | ||
| "#; |
There was a problem hiding this comment.
style: Mock server uses hardcoded 2-second timeout which could cause flaky tests on slower systems
Prompt To Fix With AI
This is a comment left during a code review.
Path: src-tauri/src/backend.rs
Line: 164:172
Comment:
**style:** Mock server uses hardcoded 2-second timeout which could cause flaky tests on slower systems
How can I resolve this? If you propose a fix, please make it concise.![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
🧹 Nitpick comments (17)
dev.sh (1)
25-53: Consider adding timeout messaging and log retention option.The backend startup and port detection logic is sound, but consider these improvements:
- The 15-second timeout (30 iterations × 0.5s) is reasonable but not communicated to the user during the wait
- The
/tmp/backend.logis useful for debugging but always deleted on exit, which might hide issues if the script crashesConsider this enhancement:
# Wait and capture the dynamic port echo -e "${YELLOW}Waiting for backend to assign port...${NC}" BACKEND_PORT="" for i in {1..30}; do if grep -q '\[BACKEND_PORT\]' /tmp/backend.log 2>/dev/null; then BACKEND_PORT=$(grep '\[BACKEND_PORT\]' /tmp/backend.log | head -1 | sed 's/.*\[BACKEND_PORT\]//') break fi + # Show progress every 2 seconds + if [ $((i % 4)) -eq 0 ]; then + echo -e "${YELLOW}Still waiting... (${i}/30)${NC}" + fi sleep 0.5 doneAnd optionally preserve the log on error:
if [ -z "$BACKEND_PORT" ]; then echo -e "${RED}✗ Failed to capture backend port${NC}" + echo -e "${YELLOW}Backend log saved to: /tmp/backend.log${NC}" cat /tmp/backend.log - exit 1 + # Don't clean up log on error (override trap for this case) + trap - EXIT + exit 1 fisrc/services/socket.ts (1)
48-51: Dynamic import for base URL resolution works correctly.The socket service properly resolves the backend URL dynamically by importing
getBaseURLat runtime. Using dynamic import here is a reasonable choice to avoid potential circular dependencies or to ensure the config is loaded when needed.Minor note: The comment on line 48 could be clarified to explain why dynamic import is used instead of static import.
Consider adding a clarifying comment:
- // 1. Get socket path from backend (using dynamic port) + // 1. Get socket path from backend (using dynamic port) + // Note: Dynamic import used to avoid circular dependencies with api.config const { getBaseURL } = await import('../config/api.config');src/hooks/useFileChanges.ts (1)
26-75: Consider parallelizing independent fetch operations.The async IIFE correctly implements dynamic base URL resolution. However, the PR status and dev servers fetches (lines 32-51) execute sequentially, which could slow down data loading.
Since these are independent operations, consider using
Promise.all()to parallelize them:(async () => { const baseURL = await getBaseURL(); - // Load PR status - fetch(`${baseURL}/workspaces/${workspaceId}/pr-status`) - .then(res => res.json()) - .then(data => { - setPrStatus(data); - }) - .catch(err => { - console.error('Failed to load PR status:', err); - setPrStatus(null); - }); - - // Load dev servers - fetch(`${baseURL}/workspaces/${workspaceId}/dev-servers`) - .then(res => res.json()) - .then(data => { - setDevServers(data.servers || []); - }) - .catch(err => { - console.error('Failed to load dev servers:', err); - setDevServers([]); - }); + // Load PR status and dev servers in parallel + await Promise.allSettled([ + fetch(`${baseURL}/workspaces/${workspaceId}/pr-status`) + .then(res => res.json()) + .then(data => setPrStatus(data)) + .catch(err => { + console.error('Failed to load PR status:', err); + setPrStatus(null); + }), + fetch(`${baseURL}/workspaces/${workspaceId}/dev-servers`) + .then(res => res.json()) + .then(data => setDevServers(data.servers || [])) + .catch(err => { + console.error('Failed to load dev servers:', err); + setDevServers([]); + }) + ]); // Check cache first for file changes if (fileChangesCache.current[workspaceId]) {backend/dev.sh (1)
14-20: Consider waiting for backend readiness instead of fixed sleep.The 2-second sleep (line 20) is arbitrary and may be insufficient if the backend takes longer to start, or wasteful if it starts quickly.
Consider parsing the backend logs for the
[BACKEND_PORT]marker to confirm readiness:# Start backend in background echo -e "${GREEN}📡 Starting backend...${NC}" -npm run dev:backend & +npm run dev:backend 2>&1 | tee /dev/tty | grep -m1 "BACKEND_PORT" & BACKEND_PID=$! -# Wait for backend to start -sleep 2 +# Wait for backend to emit port (with timeout) +timeout 10 grep -m1 "BACKEND_PORT" <(npm run dev:backend 2>&1) || echo "Warning: Backend may not be ready"Note: This is a rough example; you may need to adjust the log capture mechanism.
test-end-to-end.sh (3)
41-51: Make curl calls fail-fast and bounded.Use -fSsi (fail on HTTP errors), tighter timeouts, and reuse a CURL_OPTS var.
+CURLOPTS=(-fsS --max-time 8 --connect-timeout 3) @@ -HEALTH=$(curl -s http://localhost:$BACKEND_PORT/api/health) +HEALTH=$(curl "${CURLOPTS[@]}" http://localhost:$BACKEND_PORT/api/health || true) @@ -STATS=$(curl -s http://localhost:$BACKEND_PORT/api/stats) +STATS=$(curl "${CURLOPTS[@]}" http://localhost:$BACKEND_PORT/api/stats || true) @@ -WORKSPACES=$(curl -s http://localhost:$BACKEND_PORT/api/workspaces) +WORKSPACES=$(curl "${CURLOPTS[@]}" http://localhost:$BACKEND_PORT/api/workspaces || true) @@ -SESSIONS=$(curl -s http://localhost:$BACKEND_PORT/api/sessions) +SESSIONS=$(curl "${CURLOPTS[@]}" http://localhost:$BACKEND_PORT/api/sessions || true) @@ -FRONTEND=$(curl -s http://localhost:1420) +FRONTEND=$(curl "${CURLOPTS[@]}" http://localhost:1420 || true) @@ -SIDECAR=$(curl -s http://localhost:$BACKEND_PORT/api/sidecar/status) +SIDECAR=$(curl "${CURLOPTS[@]}" http://localhost:$BACKEND_PORT/api/sidecar/status || true)Also applies to: 55-66, 69-79, 82-92, 95-104, 107-118
59-66: Parse JSON with jq when available; keep grep fallback.Current grep/sed parsing is brittle to spacing/ordering. Prefer jq when installed.
-if echo "$STATS" | grep -q '"workspaces"'; then - WORKSPACE_COUNT=$(echo "$STATS" | grep -o '"workspaces":[0-9]*' | cut -d: -f2) - MESSAGE_COUNT=$(echo "$STATS" | grep -o '"messages":[0-9]*' | cut -d: -f2) +if command -v jq >/dev/null 2>&1; then + if echo "$STATS" | jq -e '.workspaces and .messages' >/dev/null; then + WORKSPACE_COUNT=$(echo "$STATS" | jq -r '.workspaces') + MESSAGE_COUNT=$(echo "$STATS" | jq -r '.messages') + else + echo " ❌ Stats query failed (invalid JSON)" + exit 1 + fi +elif echo "$STATS" | grep -q '"workspaces"'; then + WORKSPACE_COUNT=$(echo "$STATS" | grep -o '"workspaces":[0-9]\+' | cut -d: -f2) + MESSAGE_COUNT=$(echo "$STATS" | grep -o '"messages":[0-9]\+' | cut -d: -f2) @@ -if echo "$SESSIONS" | grep -q 'session_id\|workspace_id\|id'; then - SESSION_COUNT=$(echo "$SESSIONS" | grep -o '"id"' | wc -l | tr -d ' ') +if command -v jq >/dev/null 2>&1; then + if echo "$SESSIONS" | jq -e 'type=="array"' >/dev/null; then + SESSION_COUNT=$(echo "$SESSIONS" | jq 'length') + else + SESSION_COUNT=0 + fi +elif echo "$SESSIONS" | grep -q 'session_id\|workspace_id\|id'; then + SESSION_COUNT=$(echo "$SESSIONS" | grep -o '"id"' | wc -l | tr -d ' ') @@ -if echo "$SIDECAR" | grep -q '"running":true'; then - SOCKET=$(echo "$SIDECAR" | grep -o '"socketPath":"[^"]*"' | cut -d: -f2- | tr -d '"') +if command -v jq >/dev/null 2>&1; then + if echo "$SIDECAR" | jq -e '.running==true and .socketPath' >/dev/null; then + SOCKET=$(echo "$SIDECAR" | jq -r '.socketPath') + else + echo " ❌ Sidecar not running" + exit 1 + fi +elif echo "$SIDECAR" | grep -q '"running":true'; then + SOCKET=$(echo "$SIDECAR" | grep -o '"socketPath":"[^"]*"' | cut -d: -f2- | tr -d '"')Also applies to: 73-79, 86-92, 112-118
95-104: Make frontend port configurable.Extract 1420 into FRONTEND_PORT with env/arg override to reduce duplication.
-echo "🌐 Frontend: http://localhost:1420" +FRONTEND_PORT="${FRONTEND_PORT:-1420}" +echo "🌐 Frontend: http://localhost:$FRONTEND_PORT" @@ -FRONTEND=$(curl -s http://localhost:1420) +FRONTEND=$(curl "${CURLOPTS[@]}" http://localhost:$FRONTEND_PORT || true) @@ -echo "✅ Frontend UI: http://localhost:1420" +echo "✅ Frontend UI: http://localhost:$FRONTEND_PORT"BUILD_SYSTEM_ANALYSIS.md (2)
158-166: Specify the exact stdout marker format and provide a CI snippet.Document the required format “[BACKEND_PORT]12345” and add a ready-to-copy check to prevent regressions.
-# Option 1: Read from backend logs -BACKEND_PORT=$(grep '\[BACKEND_PORT\]' /tmp/backend.log | head -1 | sed 's/.*\[BACKEND_PORT\]//') +# Option 1: Read from backend logs (expects lines like: [BACKEND_PORT]12345) +BACKEND_PORT="$(grep '\[BACKEND_PORT\]' /tmp/backend.log | tail -1 | sed -n 's/^.*\[BACKEND_PORT\]\s*\([0-9]\{2,5\}\).*$/\1/p')" + +# CI guard (Linux): +# rg -n --hidden '^\[BACKEND_PORT\][0-9]{2,5}$' backend/server.cjs
139-147: Add note about Tauri v2 capabilities and plugin permissions.Since v2 enforces capabilities, add a reminder to align tauri.conf.json with core: permissions.
Based on learnings
src/services/api.ts (2)
57-60: Build URLs safely (avoid double /api and malformed paths).String concatenation can produce errors if endpoint includes '/api' or missing slashes. Normalize and use URL joining.
- const baseURL = await getBaseURL(); - const url = `${baseURL}${endpoint}`; + const base = await getBaseURL(); + const normalizedBase = base.endsWith('/') ? base : `${base}/`; + const normalizedPath = endpoint.replace(/^\/+/, ''); + const url = new URL(normalizedPath, normalizedBase).toString();
63-68: Consider adding Accept header and handling 204/empty JSON explicitly.Small robustness wins: send Accept: application/json; treat 204 as null without relying on content-type.
- const response = await fetch(url, { + const response = await fetch(url, { ...options, - signal: controller.signal, + signal: controller.signal, + headers: { + Accept: 'application/json', + ...(options.headers || {}), + }, }); @@ - const contentType = response.headers.get('content-type'); - if (!contentType || !contentType.includes('application/json')) { + if (response.status === 204) { + return null as T; + } + const contentType = response.headers.get('content-type') || ''; + if (!contentType.toLowerCase().includes('application/json')) { return null as T; }Also applies to: 71-96
src-tauri/src/backend.rs (3)
64-71: Trim and parse port robustly.Handle incidental whitespace after the marker before parsing to u16.
- if line.starts_with("[BACKEND_PORT]") { - if let Some(port_str) = line.strip_prefix("[BACKEND_PORT]") { - if let Ok(port_num) = port_str.parse::<u16>() { + if let Some(rest) = line.strip_prefix("[BACKEND_PORT]") { + let port_str = rest.trim(); + if let Ok(port_num) = port_str.parse::<u16>() { let mut port = port_clone.lock().unwrap(); *port = Some(port_num); println!("[BACKEND] Detected port: {}", port_num); - } - } - } + } + }
80-92: Expose detected port from start() to avoid polling in callers.Consider returning the detected port (Option) from start() alongside Result, so callers don’t need to spin-wait.
You could change signature to pub fn start(&self, backend_path: PathBuf) -> Result<Option> and return self.get_port() after the wait loop.
107-116: Reader thread lifecycle on stop.Stopping kills the child; the reader thread will exit when stdout closes. Optional: store JoinHandle and join during stop to reduce stray thread lifetime.
IMPLEMENTATION_SUMMARY.md (3)
1-50: Consider marking this as living documentation.This file captures a snapshot of implementation status at a specific point in time (test results, build outputs, performance metrics). As the codebase evolves, these assertions may become stale or misleading. Consider adding a note at the top that this is a living document requiring periodic updates, or moving time-sensitive metrics to a dedicated performance benchmarking file.
253-275: Refine readiness claims given incomplete testing.The summary uses assertive language ("Ship it!", "Ready: ✅ Yes!") while the quality checklist (lines 255-262) explicitly marks manual E2E and production deployment as incomplete. This creates a messaging contradiction—readers may assume the feature is fully validated when critical testing remains.
Consider revising the tone to "Ready for testing phase" or adding a note that completion depends on successful manual E2E validation per TESTING_GUIDE.md.
166-175: Performance metrics should note test conditions and disclaimers.The performance table (lines 168-173) presents measurements as established fact without caveats about test environment, hardware, or whether these are averages/medians/single runs. If these are one-time measurements, add a note like "measured on [OS/hardware]; may vary by system." If averages, note sample size.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (31)
.mcp.json(1 hunks)BROWSER_TEST_RESULTS.md(1 hunks)BUILD_SYSTEM_ANALYSIS.md(1 hunks)IMPLEMENTATION_SUMMARY.md(1 hunks)PORT_ARCHITECTURE_ANALYSIS.md(1 hunks)TESTING_GUIDE.md(1 hunks)backend/ISSUE_REPORT.md(1 hunks)backend/dev.sh(1 hunks)backend/server.cjs(2 hunks)conductor-dev-desktop.sh(2 hunks)conductor-setup.sh(1 hunks)dev.sh(2 hunks)package.json(0 hunks)setup-and-build.sh(1 hunks)src-tauri/DYNAMIC_PORT_IMPLEMENTATION.md(1 hunks)src-tauri/sidecar/index.cjs(2 hunks)src-tauri/src/backend.rs(3 hunks)src-tauri/src/commands.rs(2 hunks)src-tauri/src/main.rs(3 hunks)src/Dashboard.tsx(8 hunks)src/Settings.tsx(4 hunks)src/config/api.config.ts(1 hunks)src/hooks/useDashboardData.ts(5 hunks)src/hooks/useDiffStats.ts(4 hunks)src/hooks/useFileChanges.ts(2 hunks)src/hooks/useMessages.ts(4 hunks)src/hooks/useWorkspaces.ts(2 hunks)src/services/api.ts(2 hunks)src/services/socket.ts(1 hunks)src/vite-env.d.ts(1 hunks)test-end-to-end.sh(6 hunks)
💤 Files with no reviewable changes (1)
- package.json
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{tsx,jsx,ts,js}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{tsx,jsx,ts,js}: Prefer Tailwind utility classes over custom CSS for styling
Default padding is 16px; in Tailwind prefer spacing scale that maps to 16px (e.g., p-4) unless specified otherwise
Use consistent font sizes via Tailwind theme (e.g., text-sm/md/lg) instead of ad-hoc sizes
When using Framer Motion, prefer transform-based animation over x/y to ensure hardware acceleration
Default to spring animations in Framer Motion; avoid bouncy springs except for drag gestures
Files:
src/hooks/useDiffStats.tssrc/hooks/useMessages.tssrc/services/socket.tssrc/hooks/useFileChanges.tssrc/vite-env.d.tssrc/Dashboard.tsxsrc/config/api.config.tssrc/services/api.tssrc/Settings.tsxsrc/hooks/useDashboardData.tssrc/hooks/useWorkspaces.ts
**/*.{css,scss,tsx,jsx,ts,js}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{css,scss,tsx,jsx,ts,js}: Avoid hardcoding colors; use Tailwind theme tokens or CSS variables instead
Keep animations fast: default to ease-out; typical duration 200–300ms; never exceed 1s unless illustrative
Only use built-in CSS easings 'ease' or 'linear'; otherwise use the provided cubic-bezier curves per use case; avoid ease-in generally
Files:
src/hooks/useDiffStats.tssrc/hooks/useMessages.tssrc/services/socket.tssrc/hooks/useFileChanges.tssrc/vite-env.d.tssrc/Dashboard.tsxsrc/config/api.config.tssrc/services/api.tssrc/Settings.tsxsrc/hooks/useDashboardData.tssrc/hooks/useWorkspaces.ts
🧬 Code graph analysis (13)
src-tauri/sidecar/index.cjs (1)
backend/server.cjs (1)
server(712-729)
src/hooks/useDiffStats.ts (1)
src/config/api.config.ts (1)
getBaseURL(67-70)
src/hooks/useMessages.ts (1)
src/config/api.config.ts (1)
getBaseURL(67-70)
backend/dev.sh (1)
src-tauri/src/pty.rs (1)
kill(136-146)
src/services/socket.ts (1)
src/config/api.config.ts (1)
getBaseURL(67-70)
src/hooks/useFileChanges.ts (2)
src/config/api.config.ts (1)
getBaseURL(67-70)backend/server.cjs (1)
workspaceId(453-453)
backend/server.cjs (2)
src-tauri/sidecar/index.cjs (1)
server(268-268)backend/lib/database.cjs (1)
DB_PATH(26-29)
src/Dashboard.tsx (1)
src/config/api.config.ts (1)
getBaseURL(67-70)
src/services/api.ts (1)
src/config/api.config.ts (2)
API_CONFIG(72-76)getBaseURL(67-70)
src/Settings.tsx (1)
src/config/api.config.ts (1)
getBaseURL(67-70)
src/hooks/useDashboardData.ts (1)
src/config/api.config.ts (1)
getBaseURL(67-70)
src-tauri/src/main.rs (5)
src-tauri/src/backend.rs (1)
new(21-26)src-tauri/src/socket.rs (1)
new(28-33)src-tauri/src/pty.rs (1)
new(28-33)backend/lib/config.cjs (1)
commands(161-161)src-tauri/src/commands.rs (1)
get_backend_port(104-110)
src/hooks/useWorkspaces.ts (1)
src/config/api.config.ts (1)
getBaseURL(67-70)
🔇 Additional comments (36)
src/hooks/useMessages.ts (1)
2-2: LGTM! Dynamic base URL resolution implemented correctly.The hook now consistently uses
await getBaseURL()for all HTTP calls, properly replacing the hardcoded port 3333. The async pattern is correctly applied across all fetch operations.Also applies to: 6-6, 58-58, 64-64, 135-135, 159-159
TESTING_GUIDE.md (1)
1-269: Excellent testing documentation!This guide provides comprehensive coverage of the dynamic port implementation testing process, including manual test scenarios, debugging steps, and clear success/failure indicators. The structured approach with test checklists and result templates will be valuable for validation.
PORT_ARCHITECTURE_ANALYSIS.md (1)
1-259: Comprehensive architectural analysis!This document effectively explains the anti-patterns of hardcoded ports and provides well-reasoned alternatives with concrete implementation examples. The phased migration approach and comparison table are particularly helpful for understanding the trade-offs.
BROWSER_TEST_RESULTS.md (1)
1-175: Clear test results documentation!This document effectively captures the test results and clearly explains the expected behavior differences between Tauri app mode and web dev mode. The architecture flow diagrams are particularly helpful for understanding why certain limitations exist.
src-tauri/DYNAMIC_PORT_IMPLEMENTATION.md (1)
1-189: Thorough implementation documentation!This document provides excellent coverage of the dynamic port implementation, including code changes, test results, architecture flow, and troubleshooting guidance. The before/after comparisons and benefits table effectively demonstrate the value of the changes.
dev.sh (1)
60-60: LGTM! Dynamic port passed correctly to frontend.The script correctly captures the dynamic backend port and passes it to the frontend via the
VITE_BACKEND_PORTenvironment variable. This aligns well with the frontend's dynamic port resolution implementation.src/hooks/useWorkspaces.ts (1)
2-2: LGTM! Clean migration to dynamic base URL.The async base URL resolution is correctly applied to both endpoints, maintaining the existing error handling flow.
Also applies to: 21-21, 26-26
src-tauri/sidecar/index.cjs (2)
22-27: LGTM! Dynamic port configuration with good fallback.The sidecar correctly reads the backend port from the environment variable set by the backend server, with a sensible fallback to port 3333. The added log statement will help with debugging.
257-257: Good fix! Keepalive message type now consistent.Changing the keepalive message type from
'keepalive'to'keep_alive'resolves the mismatch issue mentioned in the PR description.conductor-setup.sh (1)
82-86: LGTM! Clear user guidance for dynamic port workflow.The updated messaging accurately reflects the new dynamic port system and provides clear instructions for both web dev and Tauri modes.
src/hooks/useDiffStats.ts (1)
2-2: LGTM! Consistent dynamic base URL implementation.All three fetch locations (initial load, first 5, and progressive load) correctly use
await getBaseURL(), maintaining the existing progressive loading strategy and error handling.Also applies to: 21-21, 54-54, 70-70
conductor-dev-desktop.sh (1)
5-7: LGTM! Simplified script leveraging Tauri's backend management.Removing manual backend startup logic is the right approach since Tauri's BackendManager now handles this automatically with dynamic port allocation. The added notes clearly communicate this architectural change.
Also applies to: 41-44
setup-and-build.sh (1)
82-82: LGTM! Documentation reflects automatic backend startup.The updated messaging correctly indicates that the backend now starts automatically with dynamic port assignment, eliminating the need for manual startup instructions.
src-tauri/src/commands.rs (1)
4-4: LGTM! Well-implemented backend port command.The new
get_backend_portcommand follows standard Tauri patterns, correctly usesok_or_elsefor Option-to-Result conversion, and provides a clear error message when the port isn't yet available. The integration with BackendManager state is clean.Also applies to: 98-110
src/Settings.tsx (1)
4-4: LGTM! Comprehensive migration to dynamic base URL.All six API endpoints in the Settings component correctly use
await getBaseURL(), maintaining existing error handling. The implementation is consistent with the dynamic port strategy applied throughout the codebase.Also applies to: 46-46, 59-59, 64-64, 69-69, 74-74, 85-85
backend/server.cjs (2)
26-27: LGTM! Dynamic port allocation implemented correctly.The logic correctly uses the PORT environment variable when available, otherwise falls back to port 0 for OS auto-assignment. This is standard practice for dynamic port allocation.
712-729: Code implementation is correct—sidecar properly accesses backend port from environment.Verification confirms the port communication mechanism works as intended:
process.env.BACKEND_PORTis set inbackend/server.cjs:725with the actual portsrc-tauri/sidecar/index.cjs:23correctly reads the port:const BACKEND_PORT = process.env.BACKEND_PORT || '3333'- Sidecar constructs the backend URL and logs it for confirmation (
src-tauri/sidecar/index.cjs:24, 27)The design pattern of passing the port via environment variable rather than function parameters is appropriate for this use case.
src/vite-env.d.ts (1)
1-9: LGTM! Type definitions are correct.The ambient type declarations follow Vite best practices:
- Triple-slash reference to
vite/clientprovides base Vite typesVITE_BACKEND_PORTis correctly typed as optional (?) since it's only available in web dev mode- Enables type-safe access throughout the frontend codebase
src/hooks/useFileChanges.ts (1)
1-5: LGTM! Imports and migration comment are clear.The import of
getBaseURLand the explanatory comment help developers understand the dynamic port migration.src/hooks/useDashboardData.ts (3)
21-42: LGTM! Dynamic base URL resolution implemented correctly.The function properly awaits
getBaseURL()once at the start and uses it for all fetch operations within the function.
47-73: LGTM! Efficient parallel fetching with dynamic base URL.The function correctly resolves the base URL once and reuses it across all parallel workspace diff stats fetches.
78-111: LGTM! Progressive loading with correct base URL usage.The function resolves the base URL once and correctly reuses it across both immediate and delayed fetch operations.
src/config/api.config.ts (3)
13-62: LGTM! Caching and promise deduplication implemented correctly.The implementation effectively:
- Caches the port after first resolution to avoid redundant operations
- Deduplicates concurrent calls using a shared promise
- Clears the promise reference after completion to prevent memory leaks
This pattern is appropriate for port discovery that remains constant during the app lifecycle.
67-70: LGTM! Base URL construction is correct.The function properly awaits the backend port and constructs the base URL with the
/apipath.
72-76: LGTM! API_CONFIG export updated correctly.The export correctly provides
getBaseURLas an async function, enabling dynamic port resolution throughout the application.backend/dev.sh (2)
23-26: LGTM! Frontend startup is correct.The frontend is properly started in the background with PID capture for cleanup.
28-39: LGTM! Graceful shutdown implemented correctly.The cleanup function and trap ensure both processes are terminated on exit signals, and the
waitcommand keeps the script running until both processes complete.src-tauri/src/main.rs (3)
22-22: LGTM! BackendManager initialization updated for dynamic ports.The removal of the hardcoded port parameter from
BackendManager::new()correctly aligns with the dynamic port allocation strategy.
60-72: LGTM! Port detection handles timing correctly.The code properly handles the case where the port might not be immediately available after
start()returns:
- Line 62 checks if port is available via
get_port()- Provides appropriate logging for both immediate detection and pending scenarios
- Error handling ensures the app continues even if backend startup fails
83-94: LGTM! Backend port command exposed to frontend.The addition of
commands::get_backend_portto the invoke handler enables the frontend to retrieve the dynamically assigned backend port, completing the dynamic port resolution mechanism.src/Dashboard.tsx (4)
1-46: LGTM! Imports updated correctly for dynamic base URL.The import of
getBaseURLand the explanatory comment provide clear guidance for the migration to dynamic port resolution.
115-125: LGTM! Async base URL resolution in useEffect.The async IIFE correctly resolves the base URL before fetching repos, following React hooks best practices.
162-188: LGTM! Archive workspace correctly uses dynamic base URL.The function properly awaits
getBaseURL()inline (line 164) before making the API request.
193-307: LGTM! All API functions correctly use dynamic base URL.All async functions (
createWorkspace,handleFileClick,openSystemPromptEditor,saveSystemPrompt) consistently awaitgetBaseURL()before making API requests. The implementation is correct and uniform.src/services/api.ts (1)
14-16: No breaking usages found—constructor change is safe.Verification confirmed that
ApiClientis instantiated without arguments atsrc/services/api.ts:120, and no code passes abaseURLparameter. Settings.tsx usesfetch()directly withgetBaseURL()helper, not theapiClientinstance.IMPLEMENTATION_SUMMARY.md (1)
14-34: Verify test results are reproducible and current.The test results section cites specific line numbers and assertions about test passage. If this summary is committed to the repository, ensure these results remain valid after future code changes. Consider either regenerating this section automatically (e.g., via CI), or marking it as a historical snapshot with a timestamp to prevent confusion.
Comment on lines
+3
to
+8
| "browser-automation-prod-local": { | ||
| "type": "stdio", | ||
| "command": "/opt/homebrew/bin/node", | ||
| "args": [ | ||
| "/Users/zvada/Documents/BOX/claude-experiments/browser-automation-professional/dist/server/mcp-integrated-stdio.js" | ||
| ], | ||
| "env": {} | ||
| "/Users/zvada/Documents/BOX/dev-browser/dist/server/mcp-integrated-stdio.js" | ||
| ] |
There was a problem hiding this comment.
🧩 Analysis chain
Hardcoded local path in configuration.
The MCP server configuration contains a hardcoded absolute path /Users/zvada/Documents/BOX/dev-browser/... that is specific to your local machine. This will not work for other developers and may cause issues in different environments.
Consider one of these approaches:
- Add
.mcp.jsonto.gitignoreand document the configuration in a.mcp.json.examplefile - Use a relative path or environment variable instead of an absolute path
- If this is meant to be user-specific, it should not be committed to the repository
Run this script to check if .mcp.json should be gitignored:
🏁 Script executed:
#!/bin/bash
# Check if .mcp.json is in .gitignore
if grep -q "\.mcp\.json" .gitignore; then
echo "✓ .mcp.json is already in .gitignore"
else
echo "✗ .mcp.json is NOT in .gitignore - it should be added"
fi
# Check if there's an example file
if [ -f ".mcp.json.example" ]; then
echo "✓ .mcp.json.example exists"
else
echo "✗ Consider creating .mcp.json.example for documentation"
fiLength of output: 109
Add .mcp.json to .gitignore and create .mcp.json.example.
The verification confirms that .mcp.json is committed to the repository without being in .gitignore, and it contains the hardcoded absolute path /Users/zvada/Documents/BOX/dev-browser/... specific to your machine.
Required actions:
- Add
.mcp.jsonto.gitignore - Create
.mcp.json.examplewith the same structure but using placeholder paths or environment variables - Consider updating
argsto use environment variables (e.g.,process.env.MCP_SERVER_PATH) instead of hardcoded paths
This ensures other developers won't encounter broken configurations when cloning the repository.
🤖 Prompt for AI Agents
In .mcp.json around lines 3 to 8, the file is committed with a machine-specific
absolute path; add .mcp.json to .gitignore, create a .mcp.json.example next to
it that preserves the same structure but replaces absolute paths with
placeholders or env var references (e.g., "<MCP_SERVER_PATH>" or
"${MCP_SERVER_PATH}"), and update code that consumes .mcp.json (or document
usage) to prefer environment variables (process.env.MCP_SERVER_PATH) for the
args entry so developers can configure their own paths without committing
machine-specific values.
Comment on lines
+10
to
+13
| 1. **Frontend** (React + Vite) - Port 1420 | ||
| 2. **Backend** (Node.js + Express) - Port 3333 | ||
| 3. **Tauri App** (Rust) - Desktop wrapper | ||
|
|
There was a problem hiding this comment.
Align with dynamic-port architecture (avoid hardcoded 3333).
This doc still assumes a fixed 3333 backend. Update to reflect dynamic ports and show how to discover the port in dev and Tauri.
-2. **Backend** (Node.js + Express) - Port 3333
+2. **Backend** (Node.js + Express) - Dynamic port (assigned by OS)
@@
-From `src/config/api.config.ts`:
-- Base URL: `http://localhost:3333/api`
+From `src/config/api.config.ts`:
+- Base URL: resolves at runtime via getBaseURL() using the detected port
@@
-$ curl http://localhost:3333/api/health
+$ PORT="$(grep '\[BACKEND_PORT\]' /tmp/backend.log | tail -1 | sed -n 's/^.*\[BACKEND_PORT\]\s*\([0-9]\{2,5\}\).*$/\1/p')"
+$ curl "http://localhost:${PORT}/api/health"
@@
-└─────────────────┘ JSON Responses └──────────────────┘
+└─────────────────┘ JSON Responses └──────────────────┘
│
- │ SQLite
+ │ SQLite
▼Also add a note: when running the full Tauri app, obtain the port via the Tauri command (e.g., get_backend_port) rather than logs.
Also applies to: 53-58, 71-76, 88-104
🤖 Prompt for AI Agents
backend/ISSUE_REPORT.md lines 10-13 (and similarly update 53-58, 71-76, 88-104):
the document currently hardcodes backend port 3333; change the wording to
describe the dynamic-port architecture, remove fixed port numbers, and add
concise instructions for discovering the runtime port both in development (e.g.,
read from the dev server output or an env variable like
BACKEND_PORT/DEV_SERVER_PORT) and in the Tauri context (call the provided Tauri
command such as get_backend_port to obtain the port instead of parsing logs);
update any examples to use a placeholder (e.g., <BACKEND_PORT>) and add a short
note that when running the full Tauri app the port must be obtained via the
Tauri command (get_backend_port) rather than from log messages.
|
|
||
| # Trap to kill background processes on exit | ||
| trap 'kill $(jobs -p) 2>/dev/null' EXIT | ||
| trap 'kill $(jobs -p) 2>/dev/null; rm -f /tmp/backend_port.txt' EXIT |
There was a problem hiding this comment.
Remove unused file from trap.
The trap on line 21 attempts to remove /tmp/backend_port.txt, but this file is never created anywhere in the script. The script uses /tmp/backend.log instead.
Apply this diff to fix the trap:
-trap 'kill $(jobs -p) 2>/dev/null; rm -f /tmp/backend_port.txt' EXIT
+trap 'kill $(jobs -p) 2>/dev/null; rm -f /tmp/backend.log' EXIT📝 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.
Suggested change
| trap 'kill $(jobs -p) 2>/dev/null; rm -f /tmp/backend_port.txt' EXIT | |
| trap 'kill $(jobs -p) 2>/dev/null; rm -f /tmp/backend.log' EXIT |
🤖 Prompt for AI Agents
In dev.sh around line 21, the EXIT trap attempts to remove /tmp/backend_port.txt
which is never created; replace that removal with the actual temporary file used
by the script (/tmp/backend.log) so the trap reads to kill background jobs and
rm -f /tmp/backend.log on EXIT.
Comment on lines
+105
to
+137
| ### Frontend (8 files) | ||
| 1. **src/config/api.config.ts** | ||
| - Changed `BASE_URL` → `async getBaseURL()` | ||
| - Added port caching | ||
| - Added Tauri invoke integration | ||
|
|
||
| 2. **src/services/api.ts** | ||
| - Updated to call `await getBaseURL()` | ||
|
|
||
| 3. **src/services/socket.ts** | ||
| - Updated to use dynamic URL | ||
|
|
||
| 4. **src/hooks/useDashboardData.ts** | ||
| - Fixed to use `await getBaseURL()` | ||
|
|
||
| 5. **src/hooks/useWorkspaces.ts** | ||
| - Fixed API calls | ||
|
|
||
| 6. **src/hooks/useMessages.ts** | ||
| - Fixed API calls | ||
|
|
||
| 7. **src/hooks/useDiffStats.ts** | ||
| - Fixed API calls | ||
|
|
||
| 8. **src/hooks/useFileChanges.ts** | ||
| - Fixed with async IIFE pattern | ||
|
|
||
| 9. **src/Dashboard.tsx** | ||
| - Fixed API calls | ||
|
|
||
| 10. **src/Settings.tsx** | ||
| - Fixed API calls | ||
|
|
There was a problem hiding this comment.
File count inconsistency in frontend files listing.
Line 105 claims "Frontend (8 files)" but the numbered list in lines 106-137 enumerates 10 items. Please verify: are all 10 files (including useDiffStats.ts, useFileChanges.ts, Dashboard.tsx, and Settings.tsx) actually modified, or should the count be updated to 10?
🤖 Prompt for AI Agents
In IMPLEMENTATION_SUMMARY.md around lines 105 to 137, the header says "Frontend
(8 files)" but the enumerated list contains 10 items; update the count to match
the actual number of listed frontend files (change "8" to "10") or remove any
entries that are not actually modified so the header and list are consistent.
| @@ -1,12 +1,46 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Enable strict mode for safer failures.
Add -euo pipefail and a sane IFS to fail fast on errors and unset vars.
#!/bin/bash
+set -Eeuo pipefail
+IFS=$'\n\t'📝 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.
Suggested change
| #!/bin/bash | |
| #!/bin/bash | |
| set -Eeuo pipefail | |
| IFS=$'\n\t' |
🤖 Prompt for AI Agents
In test-end-to-end.sh at line 1, the script lacks strict bash options; update it
to enable fail-fast behavior by adding a set -euo pipefail and initialize a sane
IFS (e.g. IFS=$'\n\t') immediately after the #!/bin/bash shebang so the script
exits on errors, unset variables, and failing pipeline stages.
Comment on lines
+11
to
+27
| # Auto-detect backend port if not provided | ||
| if [ -z "$1" ]; then | ||
| echo "⚠️ No port specified, attempting auto-detection..." | ||
|
|
||
| # Try to detect from backend logs | ||
| if [ -f "/tmp/backend.log" ]; then | ||
| BACKEND_PORT=$(grep '\[BACKEND_PORT\]' /tmp/backend.log | tail -1 | sed 's/.*\[BACKEND_PORT\]//') | ||
| if [ -n "$BACKEND_PORT" ]; then | ||
| echo "✅ Detected backend port from logs: $BACKEND_PORT" | ||
| fi | ||
| fi | ||
|
|
||
| # Fallback to default port | ||
| if [ -z "$BACKEND_PORT" ]; then | ||
| BACKEND_PORT=3333 | ||
| echo "⚠️ Using fallback port: $BACKEND_PORT" | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
Harden port autodetection (numeric validation + robust parsing).
Validate the detected value is a 2–5 digit port; prefer stricter sed and support env BACKEND_PORT.
-# Auto-detect backend port if not provided
-if [ -z "$1" ]; then
+:# Auto-detect backend port if not provided
+if [ -z "${1:-}" ]; then
echo "⚠️ No port specified, attempting auto-detection..."
# Try to detect from backend logs
if [ -f "/tmp/backend.log" ]; then
- BACKEND_PORT=$(grep '\[BACKEND_PORT\]' /tmp/backend.log | tail -1 | sed 's/.*\[BACKEND_PORT\]//')
+ BACKEND_PORT="$(grep '\[BACKEND_PORT\]' /tmp/backend.log | tail -1 | sed -n 's/^.*\[BACKEND_PORT\]\s*\([0-9]\{2,5\}\).*$/\1/p')"
if [ -n "$BACKEND_PORT" ]; then
echo "✅ Detected backend port from logs: $BACKEND_PORT"
fi
fi
- # Fallback to default port
+ # Env override
+ if [ -z "${BACKEND_PORT:-}" ] && [ -n "${BACKEND_PORT:-${BACKEND_PORT+x}}" ]; then
+ echo "ℹ️ Using BACKEND_PORT from environment: $BACKEND_PORT"
+ fi
+
+ # Fallback to default port
if [ -z "$BACKEND_PORT" ]; then
BACKEND_PORT=3333
echo "⚠️ Using fallback port: $BACKEND_PORT"
fi
+ # Final numeric check
+ if ! [[ "$BACKEND_PORT" =~ ^[0-9]{2,5}$ ]]; then
+ echo "❌ Invalid BACKEND_PORT: '$BACKEND_PORT'"
+ exit 1
+ fi
else
BACKEND_PORT=$1
echo "✅ Using specified port: $BACKEND_PORT"
fiRun once locally to confirm the sed pattern extracts only digits.
I need to verify the proposed sed and bash patterns work correctly. Let me test these:
🏁 Script executed:
#!/bin/bash
# Test 1: Verify sed pattern extracts 2-5 digit ports correctly
echo "=== Test 1: sed pattern extraction ==="
# Test log with port marker
TEST_LOG="[2024-01-15 10:30:45] [BACKEND_PORT] 3333 [INFO]"
echo "Input: $TEST_LOG"
# Original sed (less strict)
RESULT_ORIG=$(echo "$TEST_LOG" | sed 's/.*\[BACKEND_PORT\]//')
echo "Original sed result: '$RESULT_ORIG'"
# Proposed sed (stricter with -n flag and digit capture)
RESULT_NEW=$(echo "$TEST_LOG" | sed -n 's/^.*\[BACKEND_PORT\]\s*\([0-9]\{2,5\}\).*$/\1/p')
echo "Proposed sed result: '$RESULT_NEW'"
# Test 2: Edge cases for sed
echo -e "\n=== Test 2: sed edge cases ==="
TEST_CASES=(
"[BACKEND_PORT]3333"
"[BACKEND_PORT] 3333"
"[BACKEND_PORT] 3333 "
"[BACKEND_PORT]9999 more text"
"[BACKEND_PORT]invalid"
"[BACKEND_PORT]12 (too few digits)"
"[BACKEND_PORT]123456 (too many digits)"
)
for test_case in "${TEST_CASES[@]}"; do
result=$(echo "$test_case" | sed -n 's/^.*\[BACKEND_PORT\]\s*\([0-9]\{2,5\}\).*$/\1/p')
echo "Input: '$test_case' => Output: '$result'"
done
# Test 3: Verify bash regex validation
echo -e "\n=== Test 3: bash regex validation ==="
validate_port() {
local port=$1
if [[ "$port" =~ ^[0-9]{2,5}$ ]]; then
echo "✅ Valid: $port"
else
echo "❌ Invalid: $port"
fi
}
validate_port "3333"
validate_port "80"
validate_port "65535"
validate_port "1"
validate_port "99999"
validate_port "abc"
validate_port ""
validate_port "3333 extra"
# Test 4: Environment variable override logic check
echo -e "\n=== Test 4: Environment variable override logic ==="
# The review suggests: if [ -z "${BACKEND_PORT:-}" ] && [ -n "${BACKEND_PORT:-${BACKEND_PORT+x}}" ]; then
# This appears problematic. Let's test a clearer approach
BACKEND_PORT=""
echo "Test with BACKEND_PORT unset and empty:"
if [ -z "${BACKEND_PORT:-}" ]; then
echo " - BACKEND_PORT is empty (as expected)"
fi
# Proper way to check if env var is set:
unset BACKEND_PORT
if [ -n "${BACKEND_PORT+x}" ]; then
echo " - BACKEND_PORT is set"
else
echo " - BACKEND_PORT is NOT set"
fi
BACKEND_PORT="5000"
if [ -n "${BACKEND_PORT+x}" ]; then
echo " - BACKEND_PORT is set to: $BACKEND_PORT"
fiLength of output: 913
Harden port autodetection, but fix environment override logic and sed edge case.
Testing reveals the proposed sed pattern and bash validation work well for typical cases, but there are issues:
- Sed edge case: Input
[BACKEND_PORT]123456extracts12345(truncates 6-digit numbers to first 5 digits, which may not match intent). - Env variable logic is malformed: The condition
[ -z "${BACKEND_PORT:-}" ] && [ -n "${BACKEND_PORT:-${BACKEND_PORT+x}}" ]is contradictory (checks if BACKEND_PORT is simultaneously empty AND non-empty) and uses invalid nested substitution syntax. - Typo in diff:
:# Auto-detectshould be# Auto-detect(leading colon is a no-op).
Correct the environment check to: if [ -n "${BACKEND_PORT+x}" ] && [ -z "$BACKEND_PORT" ]; then (checks if var is set from environment but empty). Better: skip the env override block entirely and rely on the final numeric validation to catch invalid values from any source. Remove the typo. Test the sed pattern against logs with 6+ digit sequences to confirm behavior matches intent.
-# Auto-detect backend port if not provided
-if [ -z "$1" ]; then
+# Auto-detect backend port if not provided
+if [ -z "${1:-}" ]; then
echo "⚠️ No port specified, attempting auto-detection..."
# Try to detect from backend logs
if [ -f "/tmp/backend.log" ]; then
- BACKEND_PORT=$(grep '\[BACKEND_PORT\]' /tmp/backend.log | tail -1 | sed 's/.*\[BACKEND_PORT\]//')
+ BACKEND_PORT="$(grep '\[BACKEND_PORT\]' /tmp/backend.log | tail -1 | sed -n 's/^.*\[BACKEND_PORT\]\s*\([0-9]\{2,5\}\).*$/\1/p')"
if [ -n "$BACKEND_PORT" ]; then
echo "✅ Detected backend port from logs: $BACKEND_PORT"
fi
fi
# Fallback to default port
if [ -z "$BACKEND_PORT" ]; then
BACKEND_PORT=3333
echo "⚠️ Using fallback port: $BACKEND_PORT"
fi
+
+ # Final numeric validation
+ if ! [[ "$BACKEND_PORT" =~ ^[0-9]{2,5}$ ]]; then
+ echo "❌ Invalid BACKEND_PORT: '$BACKEND_PORT'"
+ exit 1
+ fi
else
BACKEND_PORT=$1
echo "✅ Using specified port: $BACKEND_PORT"
fi🤖 Prompt for AI Agents
In test-end-to-end.sh around lines 11 to 27, remove the stray leading colon on
the comment, delete the malformed/contradictory environment-override block (so
we don't try to detect “set-but-empty” via broken substitutions), and change the
log-extraction sed to reliably capture the entire port token (e.g., strip
everything up to the marker then trim whitespace) so multi-digit ports aren't
truncated; finally ensure a numeric validation step after detection enforces the
value is a valid port (digits only and within range) and treat any invalid
result as absent so the fallback port 3333 is used.
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 (2)
src/Settings.tsx (2)
44-53: Fix double “/api” in settings fetch (breaks with 404).
getBaseURL()already includes/api. Remove the extra segment and avoid recomputing the base for the same call.async function loadSettings() { try { - const response = await fetch(`${await getBaseURL()}/api/settings`); + const baseURL = await getBaseURL(); + const response = await fetch(`${baseURL}/settings`); const data = await response.json(); setSettings(data); setLoading(false); } catch (error) {
77-92: Fix double “/api” in saveSetting; reuse resolved base URL.Remove extra
/apiand avoid repeated awaits.async function saveSetting(key: string, value: any) { setSaving(true); try { - await fetch(`${await getBaseURL()}/api/settings`, { + const baseURL = await getBaseURL(); + await fetch(`${baseURL}/settings`, { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ key, value }) });
♻️ Duplicate comments (3)
src/config/api.config.ts (1)
72-76: Breaking change: BASE_URL → async getBaseURL; audit all call sites.
All consumers must nowawait API_CONFIG.getBaseURL()and must NOT re-append/api. Run the checks below to catch stale or buggy usages (e.g.,/api/api/...).#!/bin/bash # Find old static usage rg -nP --type=ts --type=tsx -C1 '\bAPI_CONFIG\.BASE_URL\b|(^|[^A-Za-z0-9_])BASE_URL\b' # Find double-/api patterns rg -nP --type=ts --type=tsx -C1 '/api/api/' # Find "${getBaseURL()}/api/..." anti-pattern rg -nP --type=ts --type=tsx -C1 '\$\{\s*(?:await\s+)?getBaseURL\(\)\s*\}/api/'src/Settings.tsx (1)
60-66: Good: batched fetches with Promise.all.Parallelizing config loads is the right call; reduces time-to-ready.
src/hooks/useFileChanges.ts (1)
26-33: Nice: AbortController cleanup addresses unmount races.The abort pattern prevents state updates after unmount/workspace switch.
Also applies to: 97-103
🧹 Nitpick comments (8)
backend/dev.sh (3)
37-37: Trim whitespace from extracted port to ensure robust parsing.The sed command extracts everything after
[BACKEND_PORT], but doesn't remove trailing whitespace or newlines. If the backend log contains[BACKEND_PORT] 5000(space-separated) or has newlines,BACKEND_PORTmay include unwanted characters. Pipe the result toxargsto safely trim.- BACKEND_PORT=$(grep '\[BACKEND_PORT\]' /tmp/backend.log | head -1 | sed 's/.*\[BACKEND_PORT\]//') + BACKEND_PORT=$(grep '\[BACKEND_PORT\]' /tmp/backend.log | head -1 | sed 's/.*\[BACKEND_PORT\]//' | xargs)
44-48: Validate that extracted port is numeric.The empty check on line 44 only verifies that a value was captured, not that it's a valid port number. If the backend log format is unexpected,
BACKEND_PORTcould be non-numeric. Add a check to validate numeric format before proceeding.if [ -z "$BACKEND_PORT" ]; then echo -e "${RED}✗ Failed to capture backend port${NC}" cat /tmp/backend.log exit 1 +elif ! [[ "$BACKEND_PORT" =~ ^[0-9]+$ ]]; then + echo -e "${RED}✗ Invalid port format: $BACKEND_PORT${NC}" + exit 1 fi
28-30: Consider simplifying directory navigation for clarity.The script changes directories multiple times (cd .., cd backend, cd .. again). Since this script is meant to run from the backend directory (per line 6), consider staying in the current directory for the backend start and using a subshell for the frontend launch to make intent clearer and avoid navigating unexpectedly if the script is invoked from elsewhere.
-cd .. && PORT=0 node backend/server.cjs > /tmp/backend.log 2>&1 & -BACKEND_PID=$! -cd backend # Wait and capture the dynamic port echo -e "${YELLOW}Waiting for backend to assign port...${NC}" @@ -60,7 +59,7 @@ fi echo "" echo -e "${BLUE}Starting frontend dev server...${NC}" echo "" -cd .. && VITE_BACKEND_PORT=$BACKEND_PORT npm run dev +PORT=0 node server.cjs > /tmp/backend.log 2>&1 & +BACKEND_PID=$! +(cd .. && VITE_BACKEND_PORT=$BACKEND_PORT npm run dev)Also applies to: 64-64
.mcp.json.example (2)
3-9: Make MCP config portable; avoid absolute Node path.Hardcoding "/opt/homebrew/bin/node" breaks on non-macOS/Homebrew setups. Prefer "node" on PATH and a repo-relative script path.
Apply:
- "browser-automation-prod-local": { - "type": "stdio", - "command": "/opt/homebrew/bin/node", - "args": [ - "/path/to/your/dev-browser/dist/server/mcp-integrated-stdio.js" - ] - }, + "browser-automation-prod-local": { + "type": "stdio", + "command": "node", + "args": [ + "dev-browser/dist/server/mcp-integrated-stdio.js" + ] + },
10-13: Addtype: "stdio"and pin the NPX tool version.The "shadcn" server lacks a type; MCP stdio servers should declare
"type": "stdio". Also,"shadcn@latest"is unreproducible; pin a version to reduce supply-chain risk.- "shadcn": { - "command": "npx", - "args": ["shadcn@latest", "mcp"] - } + "shadcn": { + "type": "stdio", + "command": "npx", + "args": ["shadcn@<pinned-version>", "mcp"] + }src/config/api.config.ts (1)
29-53: Validate port ranges and guard invalid values from env/Tauri.Prevent bad ports (0/NaN/>65535) from poisoning cache; log and continue fallback sequence.
+function isValidPort(p: number): boolean { + return Number.isInteger(p) && p > 0 && p <= 65535; +} @@ - if (import.meta.env.VITE_BACKEND_PORT) { - const port = parseInt(import.meta.env.VITE_BACKEND_PORT as string, 10); - if (!isNaN(port)) { + if (import.meta.env.VITE_BACKEND_PORT) { + const port = parseInt(import.meta.env.VITE_BACKEND_PORT as string, 10); + if (!isNaN(port) && isValidPort(port)) { console.log(`[API] Using web dev backend port: ${port}`); cachedPort = port; return port; } + console.warn(`[API] Ignoring invalid VITE_BACKEND_PORT: ${import.meta.env.VITE_BACKEND_PORT}`); } @@ - try { - const port = await invoke<number>('get_backend_port'); - console.log(`[API] Using Tauri backend port: ${port}`); - cachedPort = port; - return port; + try { + const port = await invoke<number>('get_backend_port'); + if (isValidPort(port)) { + console.log(`[API] Using Tauri backend port: ${port}`); + cachedPort = port; + return port; + } + console.warn(`[API] Invalid port from Tauri: ${port}`); } catch (error) { console.error('[API] Failed to get backend port from Tauri:', error); }Also applies to: 40-48
src/hooks/useFileChanges.ts (2)
37-65: Harden fetches: check response.ok and encode workspaceId.Prevents JSON parse on non-2xx and handles special chars in IDs.
- // Load PR status - fetch(`${baseURL}/workspaces/${workspaceId}/pr-status`, { signal }) - .then(res => res.json()) + const wid = encodeURIComponent(workspaceId); + // Load PR status + fetch(`${baseURL}/workspaces/${wid}/pr-status`, { signal }) + .then(res => { if (!res.ok) throw new Error(`HTTP ${res.status}`); return res.json(); }) @@ - fetch(`${baseURL}/workspaces/${workspaceId}/dev-servers`, { signal }) - .then(res => res.json()) + fetch(`${baseURL}/workspaces/${wid}/dev-servers`, { signal }) + .then(res => { if (!res.ok) throw new Error(`HTTP ${res.status}`); return res.json(); }) @@ - fetch(`${baseURL}/workspaces/${workspaceId}/diff-files`, { signal }) - .then(res => res.json()) + fetch(`${baseURL}/workspaces/${wid}/diff-files`, { signal }) + .then(res => { if (!res.ok) throw new Error(`HTTP ${res.status}`); return res.json(); })Also applies to: 75-91
103-104: Dependency consideration: do you needdiffStatshere?
diffStatsisn’t referenced. If not intended to trigger refetches, remove it from the deps to avoid extra network calls.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (8)
.gitignore(1 hunks).mcp.json.example(1 hunks)backend/dev.sh(1 hunks)conductor-setup.sh(3 hunks)src/Settings.tsx(5 hunks)src/config/api.config.ts(1 hunks)src/hooks/useFileChanges.ts(2 hunks)src/hooks/useMessages.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/hooks/useMessages.ts
- conductor-setup.sh
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{tsx,jsx,ts,js}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{tsx,jsx,ts,js}: Prefer Tailwind utility classes over custom CSS for styling
Default padding is 16px; in Tailwind prefer spacing scale that maps to 16px (e.g., p-4) unless specified otherwise
Use consistent font sizes via Tailwind theme (e.g., text-sm/md/lg) instead of ad-hoc sizes
When using Framer Motion, prefer transform-based animation over x/y to ensure hardware acceleration
Default to spring animations in Framer Motion; avoid bouncy springs except for drag gestures
Files:
src/Settings.tsxsrc/config/api.config.tssrc/hooks/useFileChanges.ts
**/*.{css,scss,tsx,jsx,ts,js}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{css,scss,tsx,jsx,ts,js}: Avoid hardcoding colors; use Tailwind theme tokens or CSS variables instead
Keep animations fast: default to ease-out; typical duration 200–300ms; never exceed 1s unless illustrative
Only use built-in CSS easings 'ease' or 'linear'; otherwise use the provided cubic-bezier curves per use case; avoid ease-in generally
Files:
src/Settings.tsxsrc/config/api.config.tssrc/hooks/useFileChanges.ts
🧬 Code graph analysis (2)
src/Settings.tsx (1)
src/config/api.config.ts (1)
getBaseURL(67-70)
src/hooks/useFileChanges.ts (1)
src/config/api.config.ts (1)
getBaseURL(67-70)
🔇 Additional comments (1)
.gitignore (1)
69-70: Verify intent: Is.mcp.jsonbeing tracked or ignored in this PR?The PR objectives indicate that
.mcp.jsonis being modified (public config key renamed tomcpServers.browser-automation-prod-local, args path updated). If.mcp.jsonis being committed as part of this PR, adding it to.gitignorecontradicts that intent and will cause future modifications to.mcp.jsonto be untracked.Clarify whether
.mcp.jsonshould be:
- Tracked (shared project config): Remove from
.gitignoreso changes are committed.- Ignored (user-specific local dev config): Confirm no changes to
.mcp.jsonshould be part of this PR; create a template or.mcp.json.exampleinstead.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/server.cjs (1)
184-208: Consider adding input validation for settings.While the endpoint logic is correct, consider adding validation to improve robustness:
- Key format validation (e.g., alphanumeric with limited special chars)
- Value size limit to prevent potential memory issues
Apply this diff to add basic validation:
app.post('/api/settings', (req, res) => { try { const { key, value } = req.body; if (!key) { return res.status(400).json({ error: 'key is required' }); } + + // Validate key format + if (!/^[a-zA-Z0-9_.-]+$/.test(key) || key.length > 255) { + return res.status(400).json({ error: 'Invalid key format or length' }); + } // Serialize value to JSON const serializedValue = JSON.stringify(value); + + // Check serialized size (e.g., 1MB limit) + if (serializedValue.length > 1024 * 1024) { + return res.status(400).json({ error: 'Value too large (max 1MB)' }); + } // Insert or update db.prepare(`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (2)
backend/server.cjs(4 hunks)src/Settings.tsx(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{tsx,jsx,ts,js}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{tsx,jsx,ts,js}: Prefer Tailwind utility classes over custom CSS for styling
Default padding is 16px; in Tailwind prefer spacing scale that maps to 16px (e.g., p-4) unless specified otherwise
Use consistent font sizes via Tailwind theme (e.g., text-sm/md/lg) instead of ad-hoc sizes
When using Framer Motion, prefer transform-based animation over x/y to ensure hardware acceleration
Default to spring animations in Framer Motion; avoid bouncy springs except for drag gestures
Files:
src/Settings.tsx
**/*.{css,scss,tsx,jsx,ts,js}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{css,scss,tsx,jsx,ts,js}: Avoid hardcoding colors; use Tailwind theme tokens or CSS variables instead
Keep animations fast: default to ease-out; typical duration 200–300ms; never exceed 1s unless illustrative
Only use built-in CSS easings 'ease' or 'linear'; otherwise use the provided cubic-bezier curves per use case; avoid ease-in generally
Files:
src/Settings.tsx
🧬 Code graph analysis (2)
backend/server.cjs (2)
backend/lib/config.cjs (2)
settings(318-318)settings(336-336)backend/lib/database.cjs (1)
DB_PATH(26-29)
src/Settings.tsx (1)
src/config/api.config.ts (1)
getBaseURL(67-70)
🔇 Additional comments (9)
src/Settings.tsx (5)
4-4: LGTM: Import for dynamic base URL.The import correctly brings in
getBaseURLto support dynamic port resolution.
46-47: LGTM: Dynamic base URL in loadSettings.The function correctly uses
getBaseURL()to construct the settings endpoint with the dynamic backend port.
59-67: LGTM: Parallel fetching with correct endpoint paths.This implementation correctly addresses previous review comments:
- Uses
Promise.allfor concurrent fetches, improving performance over sequential awaits- Fixed the double "/api" issue by using
${baseURL}/config/...instead of${baseURL}/api/config/...- Single
getBaseURL()call shared across all parallel requestsThe dynamic port migration is correctly implemented.
81-82: LGTM: Dynamic base URL in saveSetting.The POST request correctly uses
getBaseURL()to target the settings endpoint with the dynamic backend port.
291-291: LGTM: Explicit radix in parseInt.Good practice specifying radix 10 explicitly for decimal parsing.
backend/server.cjs (4)
26-27: Dynamic port allocation implemented correctly.The change from a hardcoded port to
PORT=0(OS-assigned) aligns with the PR objectives and correctly enables dynamic port allocation.
36-43: Settings table schema is appropriate.The table structure correctly supports key-value settings storage with timestamps.
163-181: GET /api/settings implementation is correct.The endpoint properly fetches and deserializes settings with appropriate error handling.
773-790: Excellent implementation of dynamic port handling.The server correctly:
- Captures the actual OS-assigned port from
server.address().port- Logs it in machine-readable format
[BACKEND_PORT]${actualPort}for Rust/Tauri to parse- Sets
process.env.BACKEND_PORTbefore starting the sidecarThis implementation aligns perfectly with the PR objectives for dynamic port allocation and ensures seamless port discovery across all layers.
zvadaadam
added a commit
that referenced
this pull request
Feb 6, 2026
…mentation Fixes 9 out of 10 code review issues (#1-5, #7, #9-10): - Remove dead Claude transformer accumulation (Issue #1) - Add graceful fallback for full_message column detection (Issue #2) - Use process.argv[1] for bundle-safe vendor path resolution (Issue #3) - Static import of execSync instead of dynamic require (Issue #4) - Add platform support guard instead of silently mapping to Linux (Issue #5) - Add null check on child.stdout before readline (Issue #7) - Add identity check in finally block to prevent race condition (Issue #9) - Capture thread_id for Codex message correlation (Issue #10) The in-place mutation in codex-adapter.ts (Issue #6) is verified safe because blocks are JSON-serialized before reaching the frontend, making the mutation internal to adapter lifecycle and invisible to consumers. All 193 sidecar tests pass. Zero type errors. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
zvadaadam
added a commit
that referenced
this pull request
Feb 8, 2026
…mentation Fixes 9 out of 10 code review issues (#1-5, #7, #9-10): - Remove dead Claude transformer accumulation (Issue #1) - Add graceful fallback for full_message column detection (Issue #2) - Use process.argv[1] for bundle-safe vendor path resolution (Issue #3) - Static import of execSync instead of dynamic require (Issue #4) - Add platform support guard instead of silently mapping to Linux (Issue #5) - Add null check on child.stdout before readline (Issue #7) - Add identity check in finally block to prevent race condition (Issue #9) - Capture thread_id for Codex message correlation (Issue #10) The in-place mutation in codex-adapter.ts (Issue #6) is verified safe because blocks are JSON-serialized before reaching the frontend, making the mutation internal to adapter lifecycle and invisible to consumers. All 193 sidecar tests pass. Zero type errors. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
zvadaadam
added a commit
that referenced
this pull request
Feb 8, 2026
…mentation Fixes 9 out of 10 code review issues (#1-5, #7, #9-10): - Remove dead Claude transformer accumulation (Issue #1) - Add graceful fallback for full_message column detection (Issue #2) - Use process.argv[1] for bundle-safe vendor path resolution (Issue #3) - Static import of execSync instead of dynamic require (Issue #4) - Add platform support guard instead of silently mapping to Linux (Issue #5) - Add null check on child.stdout before readline (Issue #7) - Add identity check in finally block to prevent race condition (Issue #9) - Capture thread_id for Codex message correlation (Issue #10) The in-place mutation in codex-adapter.ts (Issue #6) is verified safe because blocks are JSON-serialized before reaching the frontend, making the mutation internal to adapter lifecycle and invisible to consumers. All 193 sidecar tests pass. Zero type errors. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
zvadaadam
added a commit
that referenced
this pull request
Feb 23, 2026
## Changes ### 1. Fix DevTools Expansion Bug (Tauri Native Layer) When clicking the DevTools button, the browser view would unexpectedly expand. Root cause: `_inspector.show()` docks by splitting WKWebView's superview, resizing the webview. After `detach()` moves inspector to floating window, frame isn't restored. **Fix:** Save WKWebView frame before show(), restore after detach(). - Modified: src-tauri/src/commands/webview.rs (lines 570-582) ### 2. Fix Cursor Animation Attribute Mismatch (Issue #1 - HIGH) Cursor visual effects (move, ripple, pin) were silently failing because they searched for `data-cursor-ref` but elements were marked with `data-hive-ref`. **Fix:** Replace all `data-cursor-ref` references with `data-hive-ref` to match the actual attribute used in inject-mode.ts. - Modified: src/features/browser/automation/visual-effects.ts - Line 18: JSDoc comment - Line 25: buildMoveCursorAndRippleJs() - Line 45: buildPinCursorJs() - Line 96: buildHighlightElementJs() ### 3. Add IME Composition Guard to MessageInput (Issue #2 - MEDIUM) CJK (Chinese, Japanese, Korean) users using Input Method Editors (IME) would lose their input when pressing Enter during composition, since the handler didn't check if composition was in progress. **Fix:** Add `!e.nativeEvent.isComposing` guard to Enter key handler to prevent message send during active IME composition. - Modified: src/features/session/ui/MessageInput.tsx (line 286) ### 4. Fix Element Selector Injection Exception Handling (Issue #3 - MEDIUM) When `verify_injection` script threw an exception, the catch block didn't return, causing exception object to fall through to `eval_browser_webview_with_result`, which would parse it as a result string instead of error. **Fix:** Add explicit `return;` in catch block to prevent fall-through. - Modified: src/features/browser/ui/BrowserTab.tsx (line 521) ### 5. Fix Selector Toggle State on Eval Failure (Issue #4 - MEDIUM) When element selector injection failed, `selectorActive` was set in the UI store but outside the try block, so errors during injection were silently swallowed. **Fix:** Move state update inside try block to fail-fast on injection errors. - Modified: src/features/browser/ui/BrowserTab.tsx (line 563) ### 6. Add Polling Guard to Inspect Event Drain (Issue #5 - MEDIUM) Inspect event drain was polling unconditionally every 200ms, consuming ~5 IPC calls/second unnecessarily. With multiple tabs this compounds quickly. **Fix:** Only poll when selector is active. Add `!tab.selectorActive` guard and add to dependency array. - Modified: src/features/browser/ui/BrowserTab.tsx (lines 396-458) ### 7. Replace if/else with ts-pattern (Issue #6 - LOW) Long if/else chain for event type dispatch violates CLAUDE.md requirement to use ts-pattern for discriminated union dispatch. **Fix:** Import ts-pattern and use `.with()` / `.otherwise()` for event type matching. - Modified: src/features/browser/ui/BrowserTab.tsx (lines 26, 427-444) ### 8. Fix Hover Animation Timing (Issue #7 - LOW) InspectedElementCard hover transition was `duration-150` instead of `duration-200`, violating CLAUDE.md animation standard of 200-300ms default duration. **Fix:** Update to `duration-200 ease` to match project animation guidelines. - Modified: src/features/session/ui/InspectedElementCard.tsx (line 48) ## Verification - TypeScript compilation: ✓ (pre-existing Lucide icon issue unrelated to changes) - All fixes verified and applied by parallel code-reviewer agents - Changes preserve existing functionality while fixing correctness issues - Performance improvement: Eliminates ~5 IPC calls/second from unconditional polling ## Impact - **Bug fixes:** 3 (DevTools expansion, cursor animations, IME composition) - **Correctness improvements:** 2 (exception handling, state sync) - **Performance:** Reduced IPC quota consumption from inspect event drain - **Code quality:** Full CLAUDE.md compliance (ts-pattern, animation timing) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
zvadaadam
added a commit
that referenced
this pull request
Feb 23, 2026
* feat: Fix browser inspect element pills with code quality improvements ## Changes ### 1. Fix DevTools Expansion Bug (Tauri Native Layer) When clicking the DevTools button, the browser view would unexpectedly expand. Root cause: `_inspector.show()` docks by splitting WKWebView's superview, resizing the webview. After `detach()` moves inspector to floating window, frame isn't restored. **Fix:** Save WKWebView frame before show(), restore after detach(). - Modified: src-tauri/src/commands/webview.rs (lines 570-582) ### 2. Fix Cursor Animation Attribute Mismatch (Issue #1 - HIGH) Cursor visual effects (move, ripple, pin) were silently failing because they searched for `data-cursor-ref` but elements were marked with `data-hive-ref`. **Fix:** Replace all `data-cursor-ref` references with `data-hive-ref` to match the actual attribute used in inject-mode.ts. - Modified: src/features/browser/automation/visual-effects.ts - Line 18: JSDoc comment - Line 25: buildMoveCursorAndRippleJs() - Line 45: buildPinCursorJs() - Line 96: buildHighlightElementJs() ### 3. Add IME Composition Guard to MessageInput (Issue #2 - MEDIUM) CJK (Chinese, Japanese, Korean) users using Input Method Editors (IME) would lose their input when pressing Enter during composition, since the handler didn't check if composition was in progress. **Fix:** Add `!e.nativeEvent.isComposing` guard to Enter key handler to prevent message send during active IME composition. - Modified: src/features/session/ui/MessageInput.tsx (line 286) ### 4. Fix Element Selector Injection Exception Handling (Issue #3 - MEDIUM) When `verify_injection` script threw an exception, the catch block didn't return, causing exception object to fall through to `eval_browser_webview_with_result`, which would parse it as a result string instead of error. **Fix:** Add explicit `return;` in catch block to prevent fall-through. - Modified: src/features/browser/ui/BrowserTab.tsx (line 521) ### 5. Fix Selector Toggle State on Eval Failure (Issue #4 - MEDIUM) When element selector injection failed, `selectorActive` was set in the UI store but outside the try block, so errors during injection were silently swallowed. **Fix:** Move state update inside try block to fail-fast on injection errors. - Modified: src/features/browser/ui/BrowserTab.tsx (line 563) ### 6. Add Polling Guard to Inspect Event Drain (Issue #5 - MEDIUM) Inspect event drain was polling unconditionally every 200ms, consuming ~5 IPC calls/second unnecessarily. With multiple tabs this compounds quickly. **Fix:** Only poll when selector is active. Add `!tab.selectorActive` guard and add to dependency array. - Modified: src/features/browser/ui/BrowserTab.tsx (lines 396-458) ### 7. Replace if/else with ts-pattern (Issue #6 - LOW) Long if/else chain for event type dispatch violates CLAUDE.md requirement to use ts-pattern for discriminated union dispatch. **Fix:** Import ts-pattern and use `.with()` / `.otherwise()` for event type matching. - Modified: src/features/browser/ui/BrowserTab.tsx (lines 26, 427-444) ### 8. Fix Hover Animation Timing (Issue #7 - LOW) InspectedElementCard hover transition was `duration-150` instead of `duration-200`, violating CLAUDE.md animation standard of 200-300ms default duration. **Fix:** Update to `duration-200 ease` to match project animation guidelines. - Modified: src/features/session/ui/InspectedElementCard.tsx (line 48) ## Verification - TypeScript compilation: ✓ (pre-existing Lucide icon issue unrelated to changes) - All fixes verified and applied by parallel code-reviewer agents - Changes preserve existing functionality while fixing correctness issues - Performance improvement: Eliminates ~5 IPC calls/second from unconditional polling ## Impact - **Bug fixes:** 3 (DevTools expansion, cursor animations, IME composition) - **Correctness improvements:** 2 (exception handling, state sync) - **Performance:** Reduced IPC quota consumption from inspect event drain - **Code quality:** Full CLAUDE.md compliance (ts-pattern, animation timing) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: Address PR review — ts-pattern console dispatch + comment consistency 1. Replace nested ternary on log.l with ts-pattern match() in console drain loop (BrowserTab.tsx:365). Consistent with inspect event drain at line 428 which already uses match(). CLAUDE.md requires ts-pattern for all discriminator field dispatch. 2. Fix contradictory architecture comments across 3 files: - inject/inspect-mode.ts: Remove "DUAL mechanism: PRIMARY title-channel" language and phantom serialization queue details. Buffer+drain via eval_browser_webview_with_result is the sole path. - BrowserTab.tsx: Change "FALLBACK path" to "sole delivery path". - webview.rs: Remove reference to non-existent sendViaTitle(), mark title-channel handler as backward-compat only. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: Address PR review — 7 issues across 4 files 1. **inject-mode.ts: var() scan now covers padding/gap (#3)** Renamed colorProps → varScanProps, added padding/gap so getMatchedVarDeclarations captures var() tokens for spacing props. 2. **inject-mode.ts: font-weight fallback added (#10)** Added font-weight to the fallback conditional alongside font-size and border-radius. Filters out 'normal' (CSS default) as noise. 3. **BrowserPanel.tsx: DevTools state races with async invoke (#4)** Moved handleUpdateTab into .then() so devtoolsOpen only updates after the Rust command succeeds. Prevents UI desync on failure. 4. **BrowserTab.tsx: safeListen missing .catch() (#5)** Added .catch() to the listen() promise chain to prevent unhandled rejections if Tauri listener registration fails. 5. **BrowserTab.tsx: injectionFailed not cleared on success (#7)** Changed onUpdateTab to also set injectionFailed: false after successful injection, clearing stale error indicators. 6. **webview.rs: eprintln! gated for release builds (#8)** All 4 eprintln! calls in eval_browser_webview_with_result wrapped with cfg!(debug_assertions). Prevents leaking JS content, URLs, tokens, or page console data in production. 7. **webview.rs: DevTools error propagation (#9)** open/close_browser_devtools now use Arc<Mutex<Option<String>>> to propagate null-pointer errors out of with_webview closures. Frontend receives Err() instead of silent Ok(()) when DevTools unavailable. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ci): build inject scripts before sidecar unit tests browser-templates.test.ts imports browser-utils.ts which reads dist-inject/browser-utils.js via ?raw import. The dist-inject/ directory is gitignored and must be built first. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(test): update browser-templates tests for inject refactor - Remove dead BROWSER_UTILS import (renamed to BROWSER_UTILS_SETUP) - Remove incorrect buildPressKeyJs BROWSER_UTILS test (it intentionally doesn't use browser utils — operates on document.activeElement directly) - Update assertIsIIFE to accept both classic IIFEs (function(){...})() and arrow IIFEs (() => {...})() — esbuild format: "iife" emits the latter for compiled inject scripts like VISUAL_EFFECTS_SETUP Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address code review round 3 — security, a11y, concurrency - Gate getReactProps() and getShallowInnerHTML() to local context only; external sites can leak PII/tokens via React fiber props and innerHTML - Remove 'value' from ATTR_WHITELIST (can contain passwords) - Add in-flight guards to console and inspect drain intervals to prevent overlapping async invokes when eval_browser_webview is slow - Add aria-hidden + sr-only text for injection failure indicator - Add aria-label to screenshot button - Remove duplicate comment blocks in inject-mode.ts - Fix stale "title-channel" comment in BrowserTab.tsx Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat(browser): add screenshot button + mobile viewport toggle Add two new browser panel toolbar features: 1. Screenshot button (Camera icon): captures WKWebView as JPEG via Rust IPC and attaches to chat input via CustomEvent bridge. 2. Mobile/Desktop viewport toggle (Smartphone/Monitor icon): constrains browser area to 390px (iPhone 14 logical width) centered with mx-auto. The mobile view required replacing the tab stacking strategy — absolute positioning (inset-0) didn't reliably inherit width constraints during parent restructuring, causing getBoundingClientRect() to return stale full-width values. Tabs now stack via CSS Grid ([grid-area:1/1]) which keeps them in normal document flow. A useLayoutEffect with hide → setBounds → show cycle ensures the native WKWebView repositions synchronously on toggle. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
zvadaadam
added a commit
that referenced
this pull request
Mar 2, 2026
…uncation Addresses 3 actionable findings from CodeRabbit review: 1. **Redact raw prompts from logs** (comments #2 + #5): Replace `prompt.slice(0, 80)` with `promptLength` in timing logs in both claude-handler.ts and index.ts. User prompts were being written to /tmp log files — now only the length is logged. 2. **Fix messageCount ReferenceError** (comment #3): Move `let messageCount` declaration from inside the `try` block to before it. In JS, `let` inside `try {}` is NOT accessible in `catch {}` — they are separate block scopes. This caused a ReferenceError in the catch block, masking the actual error. 3. **Safe UTF-8 truncation in Rust** (comment #6): Replace `&error[..error.len().min(80)]` with `error.chars().take(80).collect::<String>()` in socket.rs. Byte-index slicing can panic on multibyte UTF-8 boundaries. (session_id truncation left as-is — UUIDv7 is guaranteed ASCII.) Dismissed as not actionable: - Comment #4 (error redaction in session-writer): Error messages are already sanitized by classifyError(). Redacting would lose debuggability for zero security gain. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
zvadaadam
added a commit
that referenced
this pull request
Mar 2, 2026
…ocess (#163) * fix(session): implement proper cancel/stop flow with force-kill subprocess ## Problem When user clicked stop, two things failed: 1. Agent kept running — `query.interrupt()` sends graceful message but doesn't guarantee stop 2. No visual feedback — cancelled message only persisted in catch block, which never ran on interrupt ## Solution Replace `query.interrupt()` (graceful) with `query.close()` (force-kill subprocess + children): - Terminates entire process tree, no need to track individual sub-agent tasks - Synchronous operation, immediate subprocess death Add `cancelledByUser` flag to SessionState: - Set by `handleCancel` before close - Checked in post-loop path to save cancelled message + send Tauri event - Checked in catch block for early return (avoid duplicate error handling) ## Changes **Sidecar (agent cancellation):** - `handleCancel`: Use `close()` instead of `interrupt()`, set `cancelledByUser=true` - Post-loop: When `cancelledByUser`, save cancelled message to DB with `stop_reason: "cancelled"` envelope format - Catch block: Return early if `cancelledByUser` to avoid error handling path - Add TIMING instrumentation throughout for observability **Backend (cleanup dead code):** - Remove `cancelled_at` column updates from POST `/sessions/:id/stop` - Delete unused `getLatestUserMessage` function **Frontend:** - Remove `window.confirm()` blocking confirmation dialog - In AssistantTurn: When cancelled, strip sentinel message from array, put all real messages in hiddenMessages, render warning-styled "Response stopped" badge instead of summary message **Networking/IPC:** - Add RPC logging to understand cancel flow - Socket.rs: Log cancel events ## Testing - `bun run test:sidecar` — all 198 tests pass - `bun run test:backend` — all tests pass - Manual: Start session → click Stop → agent stops immediately, "Response stopped" appears, session idle - Manual: Restart app → cancelled session persists, shows "Response stopped" badge Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com> * fix: address PR review — redact prompts, fix scope bug, safe UTF-8 truncation Addresses 3 actionable findings from CodeRabbit review: 1. **Redact raw prompts from logs** (comments #2 + #5): Replace `prompt.slice(0, 80)` with `promptLength` in timing logs in both claude-handler.ts and index.ts. User prompts were being written to /tmp log files — now only the length is logged. 2. **Fix messageCount ReferenceError** (comment #3): Move `let messageCount` declaration from inside the `try` block to before it. In JS, `let` inside `try {}` is NOT accessible in `catch {}` — they are separate block scopes. This caused a ReferenceError in the catch block, masking the actual error. 3. **Safe UTF-8 truncation in Rust** (comment #6): Replace `&error[..error.len().min(80)]` with `error.chars().take(80).collect::<String>()` in socket.rs. Byte-index slicing can panic on multibyte UTF-8 boundaries. (session_id truncation left as-is — UUIDv7 is guaranteed ASCII.) Dismissed as not actionable: - Comment #4 (error redaction in session-writer): Error messages are already sanitized by classifyError(). Redacting would lose debuggability for zero security gain. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com> * fix(test): add mockDbAll to session-writer test mock reconcileStuckSessions now calls .prepare().all() for diagnostic logging before the UPDATE. The test mock only had run/get methods, causing .all() to be undefined and the function to hit the catch block. This is a test mock gap, not a production bug. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Haiku 4.5 <noreply@anthropic.com>
zvadaadam
added a commit
that referenced
this pull request
Mar 12, 2026
#5) - #2: Clear error_message/error_category in writeUserMessage() to align with sidecar's saveUserMessage() — prevents stale error fields on retry - #5: Tighten QueryInvalidateSchema resources to z.enum(QUERY_RESOURCES) instead of z.string() — catches misspelled resource names at runtime Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
zvadaadam
added a commit
that referenced
this pull request
Mar 12, 2026
* feat(events): centralized event catalog with Zod runtime validation Implement a single source of truth for all Tauri/sidecar events with compile-time type safety and runtime validation: - Add shared/events.ts with 17 event constants, 13 Zod schemas, 3 domain arrays (QUERY_RESOURCES, MUTATION_NAMES, SIDECAR_NOTIFY_EVENTS), and AppEventMap/AppEventName types - Wire Zod validation into listen() wrapper — validates payloads at Rust→TS boundary, logs drift, still delivers on failure - Fix 3 raw string emit() calls to use BROWSER_WORKSPACE_CHANGE and CHAT_INSERT constants - Consolidate message-writing to writeUserMessage() service to eliminate duplication - Remove web polling fallback code (getWorkspacesByRepoRefetchInterval, getStatsRefetchInterval) — desktop-only app - Fix type narrowing for INVALIDATION_MAP in notify route - Tighten ChatInsertSchema element variant with proper InspectElement Zod schema - Add 32 tests: events schema validation (19), listen() Zod validation (5), notify route (8) - All 286 backend + 131 frontend + 442 sidecar tests pass Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com> * docs(CLAUDE.md): document event catalog pattern and update polling guidance - Add Event Catalog section explaining shared/events.ts, AppEventMap, Zod validation, createListenerGroup, and how to add new events - Update request volume table to reflect event-driven invalidation (workspaces/stats no longer polled) - Update polling discipline to reference push-first invalidation pipeline - Update read-layer migration priorities (no longer referenced as "polled every 2s") Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address code review feedback (#3, #5, #6, #7) - #3: Log query:invalidate emit errors in backend.rs (was silent `let _ =`) - #5: Guard malformed payload in useQueryInvalidation with `?.` fallback - #6: Update CLAUDE.md — useSession is event-driven, not polled - #7: Move misplaced ChatInsert JSDoc to correct position above its schema Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: tighten QueryInvalidateSchema and clear error fields on retry (#2, #5) - #2: Clear error_message/error_category in writeUserMessage() to align with sidecar's saveUserMessage() — prevents stale error fields on retry - #5: Tighten QueryInvalidateSchema resources to z.enum(QUERY_RESOURCES) instead of z.string() — catches misspelled resource names at runtime Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Haiku 4.5 <noreply@anthropic.com>
This was referenced Mar 19, 2026
zvadaadam
added a commit
that referenced
this pull request
Apr 2, 2026
…mentation Fixes 9 out of 10 code review issues (#1-5, #7, #9-10): - Remove dead Claude transformer accumulation (Issue #1) - Add graceful fallback for full_message column detection (Issue #2) - Use process.argv[1] for bundle-safe vendor path resolution (Issue #3) - Static import of execSync instead of dynamic require (Issue #4) - Add platform support guard instead of silently mapping to Linux (Issue #5) - Add null check on child.stdout before readline (Issue #7) - Add identity check in finally block to prevent race condition (Issue #9) - Capture thread_id for Codex message correlation (Issue #10) The in-place mutation in codex-adapter.ts (Issue #6) is verified safe because blocks are JSON-serialized before reaching the frontend, making the mutation internal to adapter lifecycle and invisible to consumers. All 193 sidecar tests pass. Zero type errors. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
zvadaadam
added a commit
that referenced
this pull request
Apr 2, 2026
#5) - #2: Clear error_message/error_category in writeUserMessage() to align with sidecar's saveUserMessage() — prevents stale error fields on retry - #5: Tighten QueryInvalidateSchema resources to z.enum(QUERY_RESOURCES) instead of z.string() — catches misspelled resource names at runtime Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
zvadaadam
added a commit
that referenced
this pull request
Apr 2, 2026
## Changes ### 1. Fix DevTools Expansion Bug (Tauri Native Layer) When clicking the DevTools button, the browser view would unexpectedly expand. Root cause: `_inspector.show()` docks by splitting WKWebView's superview, resizing the webview. After `detach()` moves inspector to floating window, frame isn't restored. **Fix:** Save WKWebView frame before show(), restore after detach(). - Modified: src-tauri/src/commands/webview.rs (lines 570-582) ### 2. Fix Cursor Animation Attribute Mismatch (Issue #1 - HIGH) Cursor visual effects (move, ripple, pin) were silently failing because they searched for `data-cursor-ref` but elements were marked with `data-hive-ref`. **Fix:** Replace all `data-cursor-ref` references with `data-hive-ref` to match the actual attribute used in inject-mode.ts. - Modified: src/features/browser/automation/visual-effects.ts - Line 18: JSDoc comment - Line 25: buildMoveCursorAndRippleJs() - Line 45: buildPinCursorJs() - Line 96: buildHighlightElementJs() ### 3. Add IME Composition Guard to MessageInput (Issue #2 - MEDIUM) CJK (Chinese, Japanese, Korean) users using Input Method Editors (IME) would lose their input when pressing Enter during composition, since the handler didn't check if composition was in progress. **Fix:** Add `!e.nativeEvent.isComposing` guard to Enter key handler to prevent message send during active IME composition. - Modified: src/features/session/ui/MessageInput.tsx (line 286) ### 4. Fix Element Selector Injection Exception Handling (Issue #3 - MEDIUM) When `verify_injection` script threw an exception, the catch block didn't return, causing exception object to fall through to `eval_browser_webview_with_result`, which would parse it as a result string instead of error. **Fix:** Add explicit `return;` in catch block to prevent fall-through. - Modified: src/features/browser/ui/BrowserTab.tsx (line 521) ### 5. Fix Selector Toggle State on Eval Failure (Issue #4 - MEDIUM) When element selector injection failed, `selectorActive` was set in the UI store but outside the try block, so errors during injection were silently swallowed. **Fix:** Move state update inside try block to fail-fast on injection errors. - Modified: src/features/browser/ui/BrowserTab.tsx (line 563) ### 6. Add Polling Guard to Inspect Event Drain (Issue #5 - MEDIUM) Inspect event drain was polling unconditionally every 200ms, consuming ~5 IPC calls/second unnecessarily. With multiple tabs this compounds quickly. **Fix:** Only poll when selector is active. Add `!tab.selectorActive` guard and add to dependency array. - Modified: src/features/browser/ui/BrowserTab.tsx (lines 396-458) ### 7. Replace if/else with ts-pattern (Issue #6 - LOW) Long if/else chain for event type dispatch violates CLAUDE.md requirement to use ts-pattern for discriminated union dispatch. **Fix:** Import ts-pattern and use `.with()` / `.otherwise()` for event type matching. - Modified: src/features/browser/ui/BrowserTab.tsx (lines 26, 427-444) ### 8. Fix Hover Animation Timing (Issue #7 - LOW) InspectedElementCard hover transition was `duration-150` instead of `duration-200`, violating CLAUDE.md animation standard of 200-300ms default duration. **Fix:** Update to `duration-200 ease` to match project animation guidelines. - Modified: src/features/session/ui/InspectedElementCard.tsx (line 48) ## Verification - TypeScript compilation: ✓ (pre-existing Lucide icon issue unrelated to changes) - All fixes verified and applied by parallel code-reviewer agents - Changes preserve existing functionality while fixing correctness issues - Performance improvement: Eliminates ~5 IPC calls/second from unconditional polling ## Impact - **Bug fixes:** 3 (DevTools expansion, cursor animations, IME composition) - **Correctness improvements:** 2 (exception handling, state sync) - **Performance:** Reduced IPC quota consumption from inspect event drain - **Code quality:** Full CLAUDE.md compliance (ts-pattern, animation timing) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
zvadaadam
added a commit
that referenced
this pull request
Apr 2, 2026
…uncation Addresses 3 actionable findings from CodeRabbit review: 1. **Redact raw prompts from logs** (comments #2 + #5): Replace `prompt.slice(0, 80)` with `promptLength` in timing logs in both claude-handler.ts and index.ts. User prompts were being written to /tmp log files — now only the length is logged. 2. **Fix messageCount ReferenceError** (comment #3): Move `let messageCount` declaration from inside the `try` block to before it. In JS, `let` inside `try {}` is NOT accessible in `catch {}` — they are separate block scopes. This caused a ReferenceError in the catch block, masking the actual error. 3. **Safe UTF-8 truncation in Rust** (comment #6): Replace `&error[..error.len().min(80)]` with `error.chars().take(80).collect::<String>()` in socket.rs. Byte-index slicing can panic on multibyte UTF-8 boundaries. (session_id truncation left as-is — UUIDv7 is guaranteed ASCII.) Dismissed as not actionable: - Comment #4 (error redaction in session-writer): Error messages are already sanitized by classifyError(). Redacting would lose debuggability for zero security gain. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
zvadaadam
added a commit
that referenced
this pull request
Apr 10, 2026
…g 3 helpers: resolveTargetPath (path guard), pipeGitStderr (stderr streaming), and getInitializedAgents (agent listing), eliminating 3 instances of code duplication across 2 files (-11 net lines).
zvadaadam
added a commit
that referenced
this pull request
Apr 10, 2026
…236) * gnhf #1: Simplified query-engine.ts (the most complex backend file) by adding a requireParam helper and wsGet/repoGet closures, eliminating 70 lines of repetitive parameter extraction boilerplate across runQuery, runRequest, and runMutation. * gnhf #2: Deduplicated repos.ts and agent-server/index.ts by extracting 3 helpers: resolveTargetPath (path guard), pipeGitStderr (stderr streaming), and getInitializedAgents (agent listing), eliminating 3 instances of code duplication across 2 files (-11 net lines). * gnhf #3: Flattened the deeply nested invalidate() function in query-engine.ts by extracting pushSnapshot() and pushWorkspaceDelta() helpers, and deduplicated identical turn/cancel and session/stop handlers in agent-server/index.ts into a shared cancelAll function (-20 net lines, nesting reduced from 5 to 3 levels). * gnhf #4: Extracted groupWorkspacesByRepo helper from query-engine.ts and consolidated resolveParentBranch in git.service.ts by eliminating 3x duplicated cache-set calls and merging two structurally identical loops into one (-9 net lines, 68 insertions / 77 deletions). * fix: correct addMethod type signature to accept sync handlers and drop unnecessary async from cancelAll The addMethod signature incorrectly required Promise<unknown> return type, but 6+ handlers are synchronous. Updated to unknown | Promise<unknown> to match actual usage. Removed the async keyword from cancelAll since it doesn't await anything. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
zvadaadam
added a commit
that referenced
this pull request
Apr 12, 2026
…ith dual-write: SDK events now produce unified Parts alongside legacy events, with new and canonical events flowing through EventBroadcaster to the backend.
zvadaadam
added a commit
that referenced
this pull request
Apr 12, 2026
* hmmm * gnhf #1: Created the complete unified message transformation layer: Part types in shared/messages/, Claude Code and Codex adapters in agent-server/messages/, with 29 passing tests covering text/reasoning/tool streaming, subagent tracking, compaction, and token usage — all without breaking any of the 347 existing tests. * gnhf #2: Wired the Claude adapter into the handler's streaming loop with dual-write: SDK events now produce unified Parts alongside legacy events, with new and canonical events flowing through EventBroadcaster to the backend. * gnhf #3: Wired the Codex SDK adapter into codex-handler.ts with dual-write: created a new CodexSdkTransformer that works directly with the SDK's ThreadEvent lifecycle (item.started/updated/completed), producing unified Parts alongside legacy events, with 23 new tests all passing (371 total agent-server tests). * gnhf #4: Added backend persistence for unified Parts: PartsAccumulator collects streaming parts, persistMessagePartsFinished attaches them to the last assistant message row via UPDATE (parts TEXT column), wired into the event handler with proper invalidation — 15 new tests, all 445 backend + 371 agent-server tests passing. * gnhf #5: Created MessagePartsEnvelope schema and parseMessageParts() safe parser in shared/messages/, added parts field to the shared Message type, and aligned backend persistence serialization with the shared schema — 14 new tests, all 459 backend + 371 agent-server tests passing. * gnhf #6: Wired parsed Parts into the frontend by adding partsMap (Map<messageId, MessagePartsEnvelope>) to useSessionWithMessages, SessionContext, and SessionProvider — components can now access typed Parts via useSession().partsMap for gradual adoption of the new rendering path, with TypeScript compilation and all 830 tests passing. * Unified message & parts architecture: agent-server events, backend persistence, DB schema Complete rewrite of the agent-server event model and backend persistence layer. The agent-server now emits 7 canonical lifecycle events that flow through the backend to the frontend, replacing the legacy dual-write system. ## Agent-Server (standalone event model) - 7 lifecycle events: turn.started → message.created → part.created/delta/done → message.done → turn.completed - 3 adapters (Claude Code, Codex CLI, Codex SDK) produce identical event types - Delta streaming: O(n) bandwidth — only new tokens sent, full text on done - Message boundaries: separate messages per API call (Claude: multi-message turns, Codex: single) - includePartialMessages: true for Claude SDK streaming - Turn boundary detection with turnVersion for session reuse - Debug CLI (bun run cli:agent-server) for testing both harnesses - PartEvent type in shared/agent-events.ts shared by agent-server and backend - Removed legacy sendMessage/sendError/sendStatusChanged from production code ## Backend (persistence + event forwarding) - Separate parts table (append-only, crash-safe) — one INSERT per part.done - message.created → INSERT message row; part.done → INSERT part row; message.done → UPDATE stop_reason - Zod schemas for all 7 events in AgentEventSchema - Part events forwarded to frontend via q:event WebSocket frames - Messages query JOINs parts via attachParts() - Legacy persistence functions deleted (persistAssistantMessage, persistToolResultMessage, etc.) - Backend debug CLI (bun run cli:backend) for end-to-end persistence testing - 12 integration tests verify event → DB pipeline ## Database Schema - New parts table: id, message_id, session_id, seq, type, data (JSON), tool_call_id, tool_name - messages table: added stop_reason, removed parts JSON column ## Tests 363 agent-server + 513 backend = 876 tests passing Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: CI failures — TypeScript errors, ESLint, e2e test, integration test skip - SessionPanel: use empty string instead of null for synthetic message content - ToolPartBlock: use createElement() instead of direct call (ComponentClass compat) - E2E test: wait for canonical session.idle/session.error instead of legacy message/queryError - Integration tests: gracefully skip when better-sqlite3 is compiled for Electron ABI Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address code review findings — TS error, lifecycle gaps, streaming overlay - Remove stop_reason from synthetic Message (TS2353 fix) - Remove dead emitMessageResult from Codex turn.completed handler - Add message.done emission to finish() in both Codex adapters - Fix streaming overlay preferring stale DB parts over live deltas - Fix synthetic message seq ordering (incrementing offset) - Forward all args in prepare-commit-msg hook - Mark stale agent memory doc as resolved Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: update Codex E2E test to use canonical events instead of legacy The Codex E2E test was still waiting for legacy `message`/`queryError` notifications which are no longer emitted. Updated to wait for canonical `session.idle`/`session.error` events, matching the Claude E2E test. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs: update agent-server event model memory — remove stale findings All previously documented issues have been resolved. Rewrote to reflect current architecture: canonical lifecycle events, backend persistence, known patterns. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
zvadaadam
added a commit
that referenced
this pull request
Apr 14, 2026
…s query-engine.ts and workspaces.ts route) into shared lib/workspace-grouping.ts, and consolidated duplicate parameter-reading helpers (readString/readNumber across query-engine.ts and commands.ts) into shared lib/query-params.ts — net reduction of 41 lines with all 825 tests passing.
zvadaadam
added a commit
that referenced
this pull request
Apr 14, 2026
* gnhf #1: Extracted two helper methods in claude-adapter.ts (closeActiveParts, accumulateStreamDelta) to eliminate 5 repeated close-text/close-thinking call sites and 2 duplicate 25-line streaming delta handlers; also replaced a duplicate 15-line formatTime function in WorkspaceItem.tsx with the existing shared formatTimeAgo utility — net reduction of 49 lines with all 375 agent-server tests passing. * gnhf #2: Extracted duplicate workspace-grouping logic (85 lines across query-engine.ts and workspaces.ts route) into shared lib/workspace-grouping.ts, and consolidated duplicate parameter-reading helpers (readString/readNumber across query-engine.ts and commands.ts) into shared lib/query-params.ts — net reduction of 41 lines with all 825 tests passing. * gnhf #3: Removed unused parameter from cancellation functions across both agent handlers, extracted helper replacing 5 identical repo-lookup patterns in repos.ts, unified duplicate git progress push functions, and removed dead export — net reduction of 23 lines across 7 files with all 825 tests passing. * gnhf #4: Removed 5 dead exports/functions and un-exported 2 internal-only functions across 6 files — net reduction of 112 lines with all 825 tests passing. * gnhf #5: Removed 2 dead exported functions (getRepoInitials, getRepoColor), un-exported 3 internal-only symbols (RECENT_PROJECT_LIMIT, resolveGitProjectRoot, setLastOpenInAppId), and replaced duplicate timeAgo in AccessSection.tsx with shared formatTimeAgo — net reduction of 41 lines across 5 files with all 825 tests passing. * gnhf #6: Un-exported 14 dead Zod schema validators (6 from shared/events.ts, 8 from shared/agent-events.ts), un-exported 6 dead type aliases from shared/events.ts, and consolidated duplicate parseGitHubRepo function from gh.service.ts and deus-import.ts into shared/lib/github.ts — reducing public API surface by 20 exports and eliminating 1 duplicate function across 5 files with all 825 tests passing. * gnhf #7: Deleted 2 dead component files (OpenInDropdown 214 lines, EmptyState 39 lines), removed 3 dead API type definitions (ApiResponse, PaginatedResponse, WorkspaceQueryParams) from shared/types/api.ts, removed 4 dead session type aliases (SessionMessageEvent, SessionErrorEvent, SessionEnterPlanModeEvent, SessionStatusEvent) from shared/types/session.ts, and cleaned up 3 barrel re-export files — net reduction of 330 lines across 7 files with all 825 tests passing. * gnhf #8: Deleted 2 dead platform files (updates.ts 31 lines, listenerGroup.ts 38 lines), removed dead function from dialog.ts, removed the entire dead StatusChanged notification pipeline across 5 files (method, schema, type, constant, test builder, union), un-exported from electron barrel, and cleaned up 3 barrel re-export files — net reduction of 121 lines across 11 files with all 825 tests passing. * gnhf #9: Removed 3 dead query hooks (useStats, useUncommittedFiles, useLastTurnFiles) with their stub service methods and query keys, removed 4 dead type definitions (ChangedFilesResult, BranchInfo, PaginationParams, DevServer) and their barrel re-exports, un-exported internal-only connectToRelay function, and cleaned up 2 dead barrel re-exports (clearToken, ConnectionIllustration) — net reduction of 121 lines across 12 files with all 825 tests passing and clean tsc. * gnhf #10: Removed 6 dead visual effect builder functions (98 lines) from visual-effects.ts, un-exported 5 internal-only symbols (resolveClaudeDir, getAgentConfig, StatusPriority, StatusConfig, WorkflowStatusConfig), and removed dead barrel re-exports (createAgentEventHandler, AgentEventHandler) from agent/index.ts — net reduction of 98 lines across 5 files with all 825 tests passing and clean tsc. * gnhf #11: Consolidated inline path validation in files.ts to use shared resolveWorkspaceRelativePath from git.service.ts, and replaced 10 hand-rolled readString+throw param validation patterns in commands.ts with the existing requireParam utility — net reduction of 17 lines across 2 files with all 825 tests passing and clean tsc. * gnhf #12: Removed 211 lines of dead CSS from global.css: the entire glitch-swap effect system (6 classes, 6 @Keyframes, 2 media queries) and the empty tool-use-enter class — none referenced by any component, with all 825 tests passing. * gnhf #13: Deleted 2 dead component files (RepoGroup.tsx 102 lines, WorkspaceItem.tsx 123 lines) from repository/ui that were never rendered anywhere, removed 14 dead barrel re-exports across 4 barrel files (repository/ui, sidebar/ui, sidebar feature, session/ui/blocks), and cleaned up knip.json — net reduction of 242 lines across 7 files with all 825 tests passing and clean tsc. * gnhf #14: Deleted the dead agent-server/messages/index.ts barrel (30 lines), removed 4 dead type exports from shared/messages/types.ts (ToolLocation, ToolOutputContent, PartType, PartTypeSchema), removed dead ToolResultMap type from chat-types.ts, and cleaned up 15 dead barrel re-exports across 4 barrel files (session/hooks, session/ui, workspace/ui, shared/hooks) — net reduction of 51 lines across 8 files with all 825 tests passing and clean tsc. * gnhf #15: Removed 10 dead type aliases from shared/agent-events.ts, un-exported 2 dead Zod schemas from shared/enums.ts, removed the dead useWindowFocus hook and its useSyncExternalStore infrastructure (50 lines) from useWindowFocus.ts, and removed dead BaseToolRendererProps barrel re-export — net reduction of 62 lines across 4 files with all 825 tests passing and clean tsc. * refactor: extract backend helpers and fix typecheck * Address CodeRabbit review feedback * Fix Claude adapter message handoff
zvadaadam
added a commit
that referenced
this pull request
Apr 18, 2026
…dentity The first stab at the 10s blink fixed DeviceFrame's effect deps but missed two other places that still churned references on every poll: 1. sim-store.refresh() set streamInfo / sims / pinnedUdid unconditionally, so every 10s each of those fields got a new reference even when the content was unchanged. Anything subscribed to them via `useSimStore()` re-rendered. 2. Sidebar's "take an initial snapshot on first stream connect" effect depended on the full streamInfo object; every poll re-fired the effect's guard check. Real fix — at the store, not the consumers: - sim-store now compares new vs. previous stream/sims/pinnedUdid and reuses the old reference when content matches. No downstream sees a spurious update. - Sidebar effect now depends on `streamInfo?.udid` (primitive) instead of the object, matching DeviceFrame's pattern. Even if we later regress #1, #2 is independently safe. Served bundle: index-Cre9C4oH.js. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
zvadaadam
added a commit
that referenced
this pull request
Apr 18, 2026
* feat(device-use): Phase 1 — scaffold v2 server + frontend
Adds the foundation for device-use v2 as a standalone product: a Hono
server on Bun.serve that will later host /mcp, /ws, /api/*, and a
React viewer at /. Dev mode proxies to Vite for HMR; prod serves the
built bundle from dist/frontend.
- Hono server at packages/device-use/src/server/index.ts with /health
- React scaffold at packages/device-use/src/frontend/ (main/App/index.html)
- Vite config rooted at src/frontend, outputs to dist/frontend
- tsconfig: JSX + DOM libs
- scripts: bun run dev (server), dev:frontend (vite), build, start
Also lands docs/device-use-v2-design.md — the full design record
(15 locked architectural decisions, phased implementation plan,
draft AAP manifest for later).
Part of the AAP (Agentic Apps Protocol) effort. This PR is the
device-use standalone refactor; AAP protocol itself is a separate PR.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(device-use): Phase 2 — expand engine with xcodebuild, build wrapper, logs
Adds three new engine primitives needed for the v2 "build + run + observe"
flow, each injectable for unit tests:
- project-info.ts — parses `xcodebuild -list -json` for an .xcodeproj or
.xcworkspace; returns schemes / targets / configurations. Used by the
agent (and the viewer's scheme picker) to know what to build.
- xcodebuild.ts — `build()` wraps `xcodebuild build` with spawn so the
stdout/stderr line stream can be piped into tool-log events (for the
viewer's logs drawer and the agent's per-build trace). Supports
AbortSignal so stop-app can cancel an in-flight build. Predicts the
built .app path when a derivedDataPath is provided; `resolveAppPath()`
is the fallback that queries -showBuildSettings -json.
- logs.ts — `streamLogs()` wraps `simctl spawn <udid> log stream` with
injectable spawner. Supports bundleId + pid filtering via --predicate,
and a stop() handle for graceful teardown.
All three follow the existing engine pattern (executor/spawner as first
arg, typed errors extending DeviceUseError). 18 new unit tests — all
using fake executors/spawners, zero real subprocess deps.
Exit: bun test passes 72/72; typecheck clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(device-use): Phase 3 — REST API, MCP HTTP endpoint, WS event bus
Wires all 23 MCP tools behind a single invoker that emits tool-event
frames over WebSocket. The same invokeTool path handles REST, MCP, and
WS calls — one engine path, one trace, zero duplicated behavior.
New modules:
- state.ts — persistent JSON at {storage}/state.json (pinned UDID,
active project/scheme). Write-serialized to avoid clobbers.
- events.ts — EventBus with 200-event ring buffer + subscriber set.
Emits ToolEvent + ToolLog; reuses MCP tool-call shape (no parallel type).
- invoker.ts — central dispatcher. Every MCP/REST/WS tool call goes
through invokeTool; emits started/completed/failed with a shared id.
- tools.ts — 23 tool definitions (Zod-schema'd handlers). Each handler
can use ctx.executor / ctx.state / ctx.stream / ctx.refMap / ctx.events.
- mcp.ts — stateless MCP HTTP handler. Creates a fresh Server+Transport
per request (SDK requirement for stateless mode) and routes tool calls
through invokeTool.
- stream.ts — StreamManager owns the long-lived simbridge --stream
subprocess; exposes proxyStream() for /stream.mjpeg passthrough.
Server wires them together:
- GET /health — liveness
- GET /api/state — persisted state
- GET /api/tools — all tools + JSON schemas
- POST /api/tools/:name — invoke a tool (REST)
- GET /api/events — recent tool-event history
- GET /stream.mjpeg — MJPEG passthrough
- GET /api/stream — stream status
- ALL /mcp — MCP HTTP transport
- WS /ws — tool-event + tool-log subscribe + invoke
Deps: @modelcontextprotocol/sdk, zod-to-json-schema.
Exit verified end-to-end:
- 86 tests pass (14 new integration tests)
- typecheck clean
- curl /api/tools → 23-tool JSON
- curl /mcp tools/list + tools/call → proper SSE responses
- scripts/ws-smoke.ts → WS receives started+completed tool-event after
REST invoke, correlation id matches
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(device-use): Phase 4 — React viewer (TopBar, DeviceFrame, Sidebar, logs)
Builds out the standalone UI served at /. Works entirely through the
HTTP + WS endpoints landed in Phase 3 — no direct engine imports
from the frontend.
Components:
- TopBar: sim picker, project path input, scheme dropdown,
▶ Run button with disabled/building/failed states
- DeviceFrame: phone bezel + MJPEG stream from /stream.mjpeg;
placeholder when no sim is pinned or sim not booted;
ripple animation dispatched from tap tool-events via a Zustand
subscription (not a useEffect setState)
- Sidebar: snapshot-on-demand with clickable @ref list
(each taps the element) + live activity feed of recent tool-events
- LogsDrawer: auto-scrolled tail of tool-log frames
- Toasts: one-liner on build/install/launch completion or any failure
Stores (Zustand):
- sim-store — sims list, pinned UDID, stream info
- project-store — active project path, scheme, configuration,
build status
- activity-store — tool-event ring buffer + toast dispatch
- logs-store — tool-log ring buffer (500 lines)
Infra:
- lib/ws.ts — WS client hook with exponential reconnect.
Routes tool-event → activity-store, tool-log → logs-store.
- lib/api.ts — typed fetch wrappers around /api/tools/*.
- styles.css — minimal dark theme using the same color tokens
as the Deus IDE. No Tailwind (keeps deps light).
Exit verified:
- typecheck clean, eslint clean
- vite build succeeds (202KB JS, 5KB CSS, 43 modules)
- Hono→Vite proxy serves the page via /
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(device-use): Phase 5 — CLI cleanup (drop SDK, drop stream CLI, add serve)
The legacy SDK (fluent session builder) and CLI `stream` subcommand
were built for a stateless-CLI world. v2 replaces them with the
long-lived server, MCP endpoint, and /stream.mjpeg passthrough —
so these surfaces are dead weight.
Deleted:
- src/sdk/ (fluent `session()` builder + apps helpers)
- src/cli/stream/ (manager + viewer-html generator)
- src/cli/commands/stream.ts (CLI stream enable/disable/status)
- scripts/sdk-smoke.ts
Added:
- src/cli/commands/serve.ts — `device-use serve [--port N] [--host] [--open]`
spawns bun on the server module and inherits stdio. Works in dev
(source path) and in bundled form.
Updated:
- src/cli/index.ts — drop stream import, add serve
- scripts/build-ts.ts — drop sdk build entry; now produces
dist/cli.js + dist/engine.js
- package.json — description updated; main/exports
point at engine (primary programmatic
entry); bin unchanged
CLI surface (preserved): list, boot, shutdown, open, session, snapshot,
query, tap, swipe, type, fill, screenshot, wait-for, open-url, apps,
appstate, launch, terminate, permission, doctor, install, serve.
Exit verified:
- typecheck clean
- 86 tests pass
- `bun run cli help` lists all commands including `serve`
- `bun run cli serve --port 3101` starts server, /health → 200
- `bun run build:ts` produces dist/cli.js + dist/engine.js cleanly
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(device-use): Phase 6 — docs, AAP manifest, composite run tool
Final phase: makes device-use v2 AAP-ready and documents the shape.
- agentic-app.json — manifest at the package root. Host IDEs (Deus,
MCP-speaking IDEs) read this to launch device-use, probe readiness
via /health, embed the viewer at /, and wire agent tool calls into
/mcp. Uses the schema shape agreed in docs/device-use-v2-design.md.
- run tool (composite) — chains build → resolveAppPath → install →
launch_app in one call. Reads CFBundleIdentifier from the built
Info.plist when bundleId isn't provided. Total MCP tools now 24.
The Viewer's ▶ Run button calls build + install + launch as
separate steps today; future phases can switch to this composite
for a cleaner progress story.
- README rewritten — new architecture, how to run the server, MCP
endpoint config for Claude Desktop, full tool list.
- AGENTS.md rewritten — new layout, how to add a tool (invokeTool
path), how to add a CLI command, event shape, hard rules ("never
bypass invokeTool", "stateless CLI stays stateless", etc.).
- .gitignore — adds .device-use/ (server's per-workspace state dir).
End-to-end verified:
- typecheck clean
- 86 tests pass
- curl /health → {ok:true}
- curl /api/tools → 24 tools
- curl /mcp tools/list → 24 tools
- set_active_simulator persists via state.json
- WebSocket streams started+completed tool-events with correlation id
- Pre-flight Info.plist-read in run tool handles bundleId resolution
device-use v2 is now the standalone product the AAP will later consume.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* test(device-use): add end-to-end server harness against real simulator
scripts/e2e-server.ts spawns a fresh device-use server on port 3199,
subscribes to /ws, exercises the full data flow, and asserts event
correlation across REST, MCP, and WebSocket paths. Runs in ~25s when
xcodebuild has a warm DerivedData cache.
Covers:
1. Server lifecycle — spawn + /health probe + graceful SIGTERM.
2. Tool registry — REST /api/tools reports 24 tools.
3. MCP HTTP transport — initialize + tools/list + tools/call(get_state)
over JSON-RPC-over-SSE with per-request stateless transport.
4. Simulator lifecycle — list_devices, set_active_simulator,
boot (Shutdown → Booted).
5. Project introspection — get_project_info parses xcodebuild -list -json
and finds the TestApp scheme.
6. Composite run tool — build → resolveAppPath → install → auto-resolve
bundleId from Info.plist → launch. End-to-end in one call.
7. Snapshot — a11y tree walk, @refs assigned to interactive nodes.
8. Tap — ref-based tap through simbridge.
9. State persistence — state.json round-trip verified on disk AND
via get_state tool.
10. WS event correlation — every tool emits paired started+completed
frames with matching ids; build streams tool-log frames during
xcodebuild.
First successful run:
24 tools · 19 tool-events · 273 tool-log frames · 0 failures · 25s
Future regression: `bun scripts/e2e-server.ts` from the package dir.
Requires Xcode + an iPhone 17 Pro simulator available.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(device-use): bind server to 0.0.0.0 so IPv6 orphans can't shadow us
The server previously bound to 127.0.0.1 only. On a machine with a
stale IPv6 listener on the same port (e.g. an orphaned sim-helper
from agent-simulator — observed in the wild, PPID=1), browsers
resolving `localhost` to ::1 first would connect to the wrong server
and render the stale UI.
Binding to 0.0.0.0 (all IPv4 interfaces) means happy-eyeballs falls
back to 127.0.0.1 cleanly when ::1 has no listener, and any IPv6
orphan on the same port becomes harmless rather than a hijacker.
Root cause, not a workaround: the orphan was a real bug in older
tooling; this change makes us robust against that class of collision.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(device-use): boot + set_active_simulator now start the MJPEG stream
The stream subprocess (simbridge --stream) was only auto-starting at
server startup for a pre-pinned sim. Booting interactively via the UI
would pin + boot the sim but never start the stream — viewer stuck on
"Waiting for stream…" even though everything else worked (snapshot,
activity feed, etc.).
Fix:
- boot tool — after simctl boot succeeds, start the stream for that
UDID. Failures are logged but don't fail the boot (CLI callers can
still boot without needing a stream).
- set_active_simulator tool — if the newly-pinned sim is already
booted, start the stream too. Covers the "open viewer after sim
is already booted externally" path.
Both return the new stream info in their result so the UI doesn't
need to wait for the 10s refresh tick to pick it up.
Verified against a real sim: restart server → pinned UDID auto-boots
and streams on an assigned port → /stream.mjpeg returns
200 multipart/x-mixed-replace with frames.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(device-use): interactive tap + swipe on the stream
Mouse events on the MJPEG image now drive the simulator, not just
watch it. Click → tap(x, y). Drag → swipe(from, to) with an 8pt
threshold to distinguish the two. Cursor turns crosshair when a
clickable stream is loaded.
To map CSS pixels → simulator points, StreamManager.start() now
probes the upstream /config on ready and computes logical point
dimensions (assuming the standard 3x retina scale). StreamInfo
gains `size: { pxW, pxH, ptW, ptH }`, exposed via /api/stream.
The DeviceFrame multiplies the click's fractional position by
ptW/ptH to get point coords for the tap/swipe tools.
Ripple animations now use the dynamic ptW from streamInfo instead
of the previous hardcoded 402pt estimate, so they land on the
right spot on any device.
Every interaction routes through invokeTool → same activity feed
entries, same tool-event broadcast, same MCP trace. Zero new
invocation paths.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(device-use): use canvas + binary WS for interactive stream
Clicks on the MJPEG <img> silently did nothing — browsers don't fire
mouse events reliably on images that swap frames continuously. The
original Deus simulator viewer avoided this by rendering MJPEG frames
into a <canvas> and sending touch events as binary WebSocket frames
to simbridge's /ws (0x03 prefix + JSON payload, begin/move/end
lifecycle). Adopting that proven pattern.
Server:
- /sim-input WebSocket endpoint. Opens an upstream WS to simbridge's
/ws for the active stream and forwards binary frames one-way
(browser → simbridge). Buffers messages until upstream handshake
completes. Closes upstream when client disconnects.
- Upgrade flow attaches { kind: "events" | "sim-input", wsUrl? } to
the Bun WS data field so a single websocket handler can multiplex.
Frontend:
- DeviceFrame now renders the stream into a <canvas>. A requestAnimation
Frame loop drives ctx.drawImage(img, 0, 0) from an off-screen Image
pointed at /stream.mjpeg. Canvas fires mouse events reliably even
while frames are updating.
- Mouse events encode 0x03-prefixed TouchEventPayload (begin on
mousedown, move on drag, end on mouseup) and send normalized 0..1
coordinates via the /sim-input WS.
- Window-level mouseup catches drags that release outside the canvas.
- Ripples fire locally on mousedown — immediate UI feedback without
waiting for a server round-trip.
Verified end-to-end: tap at Maps' center (0.27, 0.20) → Maps app
launched with "Allow Location" permission dialog. scripts/touch-smoke.ts
exercises the full path.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(device-use): auto-refresh the elements sidebar after each canvas tap
User reported the refs sidebar showed stale elements: the list still
referenced the home screen after they'd opened Maps. Fix: every canvas
mouseup schedules a snapshot refresh (debounced 500ms, collapses rapid
taps into one call). The sidebar also auto-snapshots the moment the
stream first goes live, and shows the foreground app name next to the
"elements" header so the user knows what they're looking at.
- refs-store.ts — new Zustand store with refs + foreground + loading,
a refresh() action, and a scheduleRefresh(delayMs) debouncer
- DeviceFrame — calls scheduleRefresh on canvas mouseup AND on the
window-level mouseup (drag-release-off-canvas path)
- Sidebar — consumes the refs store, auto-snapshots on first stream
connect, re-snapshots after any ref tap (sidebar → canvas tap →
screen changes → refs stale otherwise)
Activity feed also gets clearer: users can see which tap was theirs
(REST via api.tap from ref clicks) vs. which came from the direct WS
/sim-input path (those bypass invokeTool and don't show in activity).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(device-use): ref-based tap uses tapEntry (id → label → coords fallback)
Ref taps were hitting the bounding-box center, which misses the hit
target for elements like switches/checkboxes where the tappable area
is not centered on the a11y frame (e.g. Maps' "Traffic" checkbox —
the a11y node's center is on the label, the switch is to the right).
Fix: use the engine's existing tapEntry helper, which tries in order:
1. tapById(identifier) — simbridge scans the a11y tree and taps the
matching element's natural hit target
2. tapByLabel(label) — same, by accessibility label
3. tap(x, y) — coordinate fallback
This matches what the CLI's `tap @eN` command already does. Ref taps
in the viewer sidebar now toggle switches, check checkboxes, and
activate controls whose hit target differs from the a11y bounds.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(device-use): hardware-button row + context-menu suppression
Adds a row of hardware buttons (Home / Lock / Vol+ / Vol−) below the
phone. Clicks send 0x04 + JSON{button} frames to simbridge's /ws via
the existing /sim-input proxy — same transport as touch, so they're
instant and don't round-trip through invokeTool.
Also:
- Cmd+H / Cmd+L keyboard shortcuts for Home / Lock (respects focus;
ignores keys while typing in inputs)
- Canvas right-click now preventDefault's (no more browser context
menu popping over the stream)
- Button press triggers the same debounced refs-snapshot refresh that
canvas taps do, so the sidebar stays in sync after going home
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* refactor(device-use): post-cleanup — remove dead code + tighten server
Mechanical cleanup after the v2 refactor, no behavior changes. 86
tests + e2e-server (18 tool events, 97 tool-log frames) still pass.
Server:
- EventBus now owns `logHandles: Map<string, LogHandle>` directly.
stream_logs / stop_logs drop the `ctx.events as unknown as …` cast
and the `_logHandles ??= new Map()` dance.
- New `toolInputSchema()` helper in tools.ts wraps the zod-to-json-
schema call with its `as any` cast. /api/tools and /mcp tools/list
both use it; removes the duplicated expression and two casts.
- WS_SUBSCRIBERS map collapsed from `{unsubscribe, ws: unknown}` to
the unsubscribe function directly. The `ws: unknown` field was
never read.
- Dropped the `void buf;` no-op in stream.ts — the simbridge stderr
handler just discards the parameter.
Frontend:
- snapshot tool now returns an explicit `foreground: string | null`
(the root Application node's label). Frontend reads it as a named
field instead of digging into `tree[0].label`.
- snapshot call in api.ts stops requesting interactiveOnly:false
(it was inflating the sidebar with non-clickable static text).
Server default of `interactiveOnly: true` is what the UI wants.
- project-store: BuildStatus type trimmed from 6 states to 4
(idle|running|done|failed). The old "building|installing|launching"
sub-states were dead — nothing set them.
- TopBar's Run button wired to `api.run()` (the composite: build →
install → launch) instead of `api.build()`. "▶ run" actually runs.
- DeviceFrame: extracted `normalize(clientX, clientY)` helper so the
canvas mouseup and window-level mouseup share one coordinate path.
- Every scheduleRefresh() callsite dropped its explicit 500ms arg;
that's now the store's default.
CLI:
- `device-use serve --open` now polls /health (up to 6s) before
opening the browser, replacing the 1500ms blind sleep.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* refactor(device-use): drop Vol+/Vol− buttons — simbridge doesn't map them
simbridge's SimAccessibilityBridge.sendButton only dispatches home and
lock; volumeup/volumedown payloads reach the handler but are no-ops.
Keeping the UI honest by removing the two buttons that don't do
anything. If volume control comes back later, it'll need a sendButton
case in Swift first.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(device-use): stop the 10s blink — use primitive deps in DeviceFrame
sim-store refreshes /api/stream every 10s and writes a fresh
streamInfo object into state. Both effects in DeviceFrame (the MJPEG
canvas render loop and the /sim-input WebSocket) depended on the
whole streamInfo object, so each poll tore down the canvas, cleared
it, recreated the Image element, and reconnected the WS. Result: a
visible black flash on the phone every 10 seconds and a one-frame
gap in touch delivery if you happened to be mid-drag.
Fix: depend on primitive identity — streamUdid (and streamPort for
the WS). Same UDID/port → same effect instance, no teardown. If the
underlying sim actually changes UDID (user switches sim), both
effects re-fire correctly.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(device-use): stop the blink at the source — sim-store preserves identity
The first stab at the 10s blink fixed DeviceFrame's effect deps but
missed two other places that still churned references on every poll:
1. sim-store.refresh() set streamInfo / sims / pinnedUdid
unconditionally, so every 10s each of those fields got a new
reference even when the content was unchanged. Anything subscribed
to them via `useSimStore()` re-rendered.
2. Sidebar's "take an initial snapshot on first stream connect"
effect depended on the full streamInfo object; every poll
re-fired the effect's guard check.
Real fix — at the store, not the consumers:
- sim-store now compares new vs. previous stream/sims/pinnedUdid
and reuses the old reference when content matches. No downstream
sees a spurious update.
- Sidebar effect now depends on `streamInfo?.udid` (primitive)
instead of the object, matching DeviceFrame's pattern. Even if
we later regress #1, #2 is independently safe.
Served bundle: index-Cre9C4oH.js.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(device-use): keyboard input + CI workflow + long-press verified
Three changes closing the last gaps before the PR opens.
1. Keyboard input on the viewer — keystrokes while focus isn't on
another input get batched into a 150ms idle buffer and flushed via
the type_text tool. Enter flushes with submit=true (presses Return
after). Backspace trims the local buffer. Modifier keys (Cmd/Ctrl)
are preserved for the existing hardware-button shortcuts
(Cmd+H / Cmd+L). Buffering collapses fast typing like "hello"
from 5 sequential simbridge subprocesses into one.
2. Long-press verified — scripts/longpress-smoke.ts holds begin for
1500ms, then releases end, and asserts the iOS context menu
appears (screen refs grow, including the "Dismiss context menu"
action). Passes against a real iPhone simulator. The architecture
already supported it: our /sim-input WS forwards begin+end raw to
simbridge, which keeps the touch down for whatever interval the
client chooses. No code needed — just proof.
3. GitHub Actions workflow for nightly e2e on macos-15. Picks any
available iPhone, boots it, runs scripts/e2e-server.ts end-to-end
(spawn → /health → WS → REST + MCP → build → install → launch →
tap → state persistence). Schedule + manual dispatch only; we
don't run per-PR (macOS minutes cost 10×). The e2e script now
honors $E2E_SIM_UDID to target a specific sim without changing
its default behavior for local dev.
Verified end-to-end:
- type_text via REST: {success:true}
- longpress-smoke.ts: refs 3 → 7, iOS context menu opened
- touch-smoke.ts: still passes
- e2e-server.ts: full flow passes (18 tool-events, 97 tool-logs)
- 86/86 unit tests pass
- typecheck clean
Out of scope for this PR (noted in design doc as v1.1):
- wheel→scroll mapping
- pinch/rotation multi-touch
- CLI actions appearing in /ws activity feed (peer model stays pure)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* docs(device-use): handoff section for the next PR (Deus AAP host)
Adds a "Handoff to next PR" block to docs/device-use-v2-design.md so
whoever picks up the Deus-side AAP integration has a working brief.
Covers:
- What to build in Deus (apps.service.ts, app-registrar.ts, lifecycle
tools, frontend launcher) with concrete file paths.
- What to delete after AAP works (~2.5k LOC of legacy simulator code
in apps/web/src/features/simulator/ + simulator-context.ts +
deus-tools/simulator.ts).
- 8 lessons device-use learned that should shape AAP host behavior:
requires validation, {port} substitution, mandatory health probe,
storage env var, child.on(exit) > polling, low-latency input
back-channel, runtime setMcpServers, framework-agnostic frontend.
- Things device-use deliberately deferred (wheel-scroll, CLI in WS,
multi-sim side-by-side) and where they belong.
- Open design questions for the next PR.
- A concrete first-commit slice for the Deus AAP PR.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(device-use): address CodeRabbit review on PR #249
17 inline comments from the bot — addressing all of them. Brief
mapping (file → comment id):
CRITICAL
package.json#62 Add missing zustand dep (was hoisted to root, would
fail bun install --frozen-lockfile in CI).
MAJOR
agentic-app.json#33 UI/MCP URLs use 127.0.0.1 instead of localhost so
host IDEs don't trip over IPv4/IPv6 happy-eyeballs
ambiguity. serve.ts also defaults --host to 0.0.0.0
(matches the server's own default), and the printed
probe URL collapses 0.0.0.0 → 127.0.0.1.
serve.ts#33 Compiled-layout candidate path was off-by-one
(..server vs ../..server). Fixed.
serve.ts#65 Browser only opens after /health responds 200; on
startup failure, no dead tab.
serve.ts#72 Propagate child exit code; non-zero exits return
{success:false} with the code.
e2e-server.ts#173 ChildProcess.killed is set when the signal is sent,
not when the process exits — using it as a kill-fallback
check would race a SIGTERM-ignoring child. Switched
to a tracked `exited` flag + clearTimeout on exit.
Sidebar.tsx#17 Refresh refs whenever streamUdid changes (sim swap),
not only when refs.length === 0. Stops stale refs
from a previous sim from lingering.
styles.css#85 Added --on-accent / --stage-bg / --frame-bg /
--canvas-bg / --shadow-* tokens; removed every
literal `white`, `#06060a`, `#1a1a22`, `#000`, and
`rgba(...)` from the stylesheet body.
state.ts#72 persist() now catches mkdir/writeFile failures
inside the chained promise so a single transient
error can't permanently brick the writeChain.
tools.ts#562 stream_logs wires onError + onExit to delete the
handle from logHandles, so failed/exited streams
don't leak.
MINOR
design-doc#57,#65 Architecture diagram said "TanStack Start on Nitro"
but the locked stack (decision #5) is "Hono on
Bun.serve" / "Vite + React". Aligned.
README.md#88 Added `text` language tag to the architecture fenced
code block (markdownlint MD040).
project-info.ts#43 detectKind() throws XcodebuildError on inputs that
aren't .xcodeproj or .xcworkspace, instead of
silently treating them as .xcodeproj.
xcodebuild.ts#159 Removed the synthesized appPath from BuildResult —
it was a guess based on `scheme.app` that's wrong
whenever target product name differs. Callers
should use resolveAppPath() (already used by the
`run` composite tool).
TopBar.tsx#15 onBoot pushes a failed tool-event into the activity
store on api.boot() failure, so the toast surfaces
the error.
sim-store.ts#67 setPinned checks res.success — only commits the
pinnedUdid change on success, otherwise stores
error state.
styles.css#345 Replaced deprecated `word-break: break-word` with
`overflow-wrap: anywhere` (modern equivalent).
Side effects:
scripts/*.ts Added `export {}` to the three smoke scripts so
tsc treats them as modules, not global scripts
(was causing duplicate-symbol errors after a
second smoke script joined the project).
Verified:
- 86/86 unit tests pass
- typecheck clean
- bun scripts/e2e-server.ts passes against a real iPhone 17 Pro
(17 tool-events, 97 tool-log frames, 0 failures)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* chore(device-use): sync bun.lock with zustand workspace dep
The zustand dep was added to packages/device-use/package.json in
f14d701 but the workspace lockfile entry wasn't captured. This is
the missing one-line lockfile sync — `bun install --frozen-lockfile`
was already passing in CI because the root entry was up-to-date,
but the workspace block needed the addition for consistency.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(device-use): address CodeRabbit round 2 (3/4)
- design-doc#168 — removed stale "TanStack Start" reference from the
dev/prod modes row; replaced with the actual setup (Hono server with
--hot, separate Vite dev server, prod = bundled frontend + same
Hono server in NODE_ENV=production).
- sim-store#19 (sameStream) — extended comparison beyond udid + port
to include url and structural size comparison (pxW/pxH/ptW/ptH).
Stream metadata changes (e.g. orientation rotation altering size
while UDID/port stay fixed) now correctly notify subscribers.
- sim-store#30 (sameSims) — added runtime to the per-simulator
equality check. Runtime upgrades now propagate.
Disagreed with one suggestion (TanStack Query refactor for sim-store
fetches). Reasoning in the round summary on the PR — short version:
device-use is a standalone package whose frontend deliberately runs
on Zustand only, has no TanStack Query dependency today, and the
guideline cited applies to apps/web (Deus's main UI), not to
packaged sub-products. Adding TanStack Query for two REST reads
would inflate the bundle and introduce a paradigm split between the
WS event subscription and the polled data, with no real benefit.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* docs(device-use): clarify launch.ready manifest path in handoff section
CodeRabbit flagged that `launch.ready.http` was an inconsistent path
shorthand — the actual manifest schema is `launch.ready` with
`{type: "http", path: "/health"}`. Reworded to use the correct
shape so future readers don't assume a `launch.ready.http` key.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* docs(device-use): address CodeRabbit round 4 (5 doc nits)
All 5 are doc-only fixes in docs/device-use-v2-design.md:
- Storage path duplication — `state.json` lives at
`{storage.workspace}/state.json`, not `{storage.workspace}/.device-use/state.json`
(the `.device-use` segment is already inside `storage.workspace`).
Fixed in 3 places (table row #12, backend wiring section).
- Architecture fence missing language — added `text` (markdownlint MD040).
- /ws bullet overstated CLI visibility — clarified that only actions
routed through the server (REST, MCP, viewer) populate the activity
stream; standalone CLI invocations bypass it (peer model).
- Phase 1 stack text was stale — said "Add TanStack Start" + "Nitro"
but the locked stack (decision #5) is Hono on Bun.serve + Vite.
Bullet rewritten to match what was actually built.
- Out-of-scope note about agentic-app.json was outdated — the manifest
IS in this PR; the next PR is the Deus host that consumes it. Reworded
+ retitled the manifest section to match.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* docs(device-use): align manifest example with shipped agentic-app.json
CodeRabbit noticed the design-doc manifest example still showed
\`localhost\` URLs while the actual \`packages/device-use/agentic-app.json\`
uses \`127.0.0.1\` (the IPv4-safe form chosen during the round-1 fix
to avoid IPv4/IPv6 happy-eyeballs ambiguity). Aligned the example so
copy/paste from the doc doesn't regress the fix.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* style(device-use): adopt Deus design tokens (Jony-Ive dark theme)
The viewer now uses the same color/text/radius/font system that
apps/web/src/global.css defines for the Deus IDE itself, so embedding
device-use as an AAP app feels native instead of looking like a
separate skin.
Tokens mirrored 1:1 from Deus's dark theme:
- Backgrounds — six-tier hierarchy (--bg-base #0b0b0b → --bg-surface
→ --bg-elevated → --bg-raised → --bg-overlay → --bg-muted #2a2a2a).
Each step adds perceptible depth without needing borders.
- Text — five-level OKLCH grayscale (--text-primary 0.86 → 0.32),
generous gaps between levels.
- Accent — `oklch(0.78 0.09 345)` (Deus's "cool rose" primary), not
the previous purple. Used for the run button, status dot glow,
ref ids, and ripples.
- Semantic signals — equiluminant OKLCH (success / warning /
destructive all at L≈0.65-0.68), matches Deus exactly.
- Borders — `color-mix(in oklch, var(--foreground) 5%, transparent)`,
semi-transparent so they adapt to any surface.
- Radius scale — 2 / 4 / 6 / 8 / 10 / 12 / 16 / 20 / 24 px.
Device frame uses --radius-4xl (24px), canvas inside uses
--radius-3xl (20px), buttons --radius-md/lg.
- Typography — 13px base (matches Deus's dense IDE scale), system
-apple-system / SF Pro Text + SF Mono, negative letter-spacing
throughout (-0.005em / -0.015em on brand) for the "premium texture"
layer.
- Custom scrollbars themed to bg-raised → bg-overlay on hover.
JSX inline styles updated to reference the new tokens
(--accent-2 → --primary, --danger → --destructive, --dim →
--text-muted-foreground).
Class names unchanged — same DOM structure, just new visual tokens
underneath. Verified:
- 86/86 tests pass
- typecheck clean
- vite build: index-CKuhJYDJ.css (9.62 KB), index-BOvIo28T.js
- bun scripts/e2e-server.ts PASS — 18 tool-events, 97 tool-log
frames, 0 failures
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(device-use): collapsible logs drawer (collapsed by default)
The logs pane was eating 220px at the bottom even when empty, which
is most of the time. Now it's just a ~32px header row by default; the
phone + sidebar get the extra vertical space back.
- Click the header row to toggle expand/collapse.
- Chevron flips ▸ / ▾ to signal state.
- Auto-expands on any new log line — if you triggered a build or a
log stream, the drawer pops open for you. Collapsing is always
manual. Auto-expand uses a direct Zustand subscription (not a
useEffect setState) to satisfy react-hooks/set-state-in-effect.
- Clear button hidden when collapsed (no affordance without content).
- Grid row for logs changed from fixed 220px to `auto`; the
`.expanded` class pins the body at 220px when open.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(device-use): themed Select (Radix UI) — replaces native <select>
Native <select> popups use OS chrome (white on macOS), breaking the
dark Deus theme. Swapped for @radix-ui/react-select wrapped in a
small themed component — same primitive Deus's apps/web Select uses,
just styled with plain CSS against our token system (no Tailwind).
What's new:
- src/frontend/components/Select.tsx — minimal wrapper exposing a
plain {value, onValueChange, options, placeholder} API. Options
can be any ReactNode so the sim picker can render a state dot + a
dim runtime suffix inline.
- Styled trigger matches the other topbar inputs (bg-elevated, 1px
border, 6px radius, hover brightens to bg-raised, focus-visible
rings with --ring).
- Portaled popover uses --popover / --border / --shadow-elevated,
radius-xl; items have 6/10 padding, left checkmark indicator in
--primary, data-highlighted lifts to bg-elevated.
- Chevron rotates 180° on open (data-state="open").
- Smooth 180ms in / 150ms out with cubic-bezier matching apps/web.
TopBar wiring:
- Sim picker renders each option as `[dot] Name · runtime`, with the
dot green+glow when that sim is Booted.
- Scheme picker renders as a plain list.
- Empty "no simulators" case stays inline text (no dropdown needed).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(device-use): correct cache-control for prod static serving
Without explicit cache directives, browsers heuristically cache the
HTML shell and keep pointing at old Vite-hashed asset URLs across
deploys — user refreshes, sees a stale bundle. This was hitting the
dropdown-themed commit: the new bundle was built + served but the
browser's cached HTML still referenced the pre-Radix build.
Fix in the prod static-serving branch:
- /assets/* → `Cache-Control: public, max-age=31536000, immutable`
(Vite fingerprints these, so the URL changes whenever the content
does — safe to cache forever).
- Everything else (including the HTML shell) → `Cache-Control: no-cache`
(revalidate every load so new bundle URLs are picked up immediately).
Verified via `curl -D -`. HTML now sends `no-cache`, asset sends
`public, max-age=31536000, immutable`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(device-use): proper iPhone/iPad device frame (ported from apps/web)
The old frame was just a rounded rect with 12px padding and 40px
radius — didn't match iPhone proportions, had no dynamic island,
no camera hole, and no accurate bezel thickness. Ported the
GenericDeviceFrame shape from
apps/web/src/features/simulator/ui/DeviceFrame.tsx exactly:
- Aspect ratio per device class (iPhone 430:932, iPad 834:1194);
the frame scales to fit the stage height while preserving ratio.
- Shell radius 3.25rem (iPhone) / 2.75rem (iPad), screen radius
2.6rem / 2.1rem — the two-layer corner system.
- Percentage-based screen insets (iPhone 1.7% top / 2.5% sides /
1.9% bottom) matching Apple's actual bezel thickness.
- Dynamic island: 38% × 4.1%, positioned at 2.35% top, bg-base
rounded-full. Shows for iPhone only.
- Camera dot: 10×10, positioned at 1.45% top with a subtle ring.
Shows for iPad only.
- Inner bezel ring at inset 0.6% — catches light, hints at edge
thickness without adding chrome.
- iPhone vs iPad detection from pinned sim's name (same heuristic
apps/web uses).
We don't bundle the Apple-derived bezel assets (apps/web loads them
from a generated manifest via scripts/prepare-device-chrome.mjs —
needs Xcode + licensing considerations for packaged device-use);
the generic shell is the fallback Deus uses anyway when the manifest
isn't present, so visual parity is the same as Deus's default.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(device-use): drop overlay dynamic island/camera dot — stream already has them
The simulator's MJPEG stream captures iOS's full screen, including the
native status bar with its own dynamic island (iPhone) or camera
cutout (iPad). My overlay versions drew a second pill on top at
slightly different sizes/positions, creating the visible "two things
stacked" artifact the user flagged.
Fix: remove the overlay divs + their CSS. The stream shows the real
ones rendered by iOS at the right size/position.
Why Deus's apps/web draws them anyway: their code path assumes the
captured stream excludes the status bar (that is the case when they
eventually swap in the Apple-derived bezel assets, which crop it).
For the generic-shell + full-screen MJPEG case (what we have), the
overlays are redundant.
Kept: shell background, proper aspect ratio, shell/screen radii,
percentage screen insets, subtle inner ring. Those still make the
frame feel intentional without faking what's already in the stream.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(device-use): shell aspect ratio derived from stream so the screen fits exactly
The hardcoded 430/932 shell AR only matches the base iPhone 16 Pro.
For iPhone Air (420×912) or iPhone 17 Pro Max (440×956), the stream
either got stretched/squished by the canvas filling the box, or left
a black gap at the top/bottom because the inner screen box didn't
match the stream's aspect ratio.
Fix: compute the shell aspect ratio from streamInfo.size.ptW/ptH
so that, after subtracting the percentage bezel insets, the inner
screen box has exactly the same ratio as the stream. The canvas then
fills the screen cleanly with no object-fit letterboxing needed.
Math:
widthFrac = 1 − (left% + right%) / 100
heightFrac = 1 − (top% + bottom%) / 100
screenAR = ptW / ptH (from streamInfo.size)
shellAR = screenAR × widthFrac / heightFrac
Bezel insets (1.7/2.5/2.5/1.9 % for iPhone, 2.1 all-around for iPad)
are unchanged. Fallback to the previous hardcoded ratios when we
don't have streamInfo yet (empty-state placeholder). Dropped the
canvas `object-fit: contain` since there's nothing to contain now —
the box matches.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* refactor(device-use): collapsible sidebar with Panel wrapper
Extract ElementsPanel and ActivityPanel as discrete components under
components/sidebar/, share a Panel wrapper for chrome, and add a
collapsible shell with a Cmd+B toggle. Sidebar state persists via a new
ui-store (localStorage). Collapsed state shows a thin rail button on
the right edge.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* docs: video-use + AAP v1.1 design draft
Working design doc covering (a) hyperframes investigation as a
candidate second AAP app and (b) AAP v1 protocol revisions
surfaced by that investigation. Not implemented yet —
informs upcoming PR sequence.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
zvadaadam
added a commit
that referenced
this pull request
May 31, 2026
…dentity The first stab at the 10s blink fixed DeviceFrame's effect deps but missed two other places that still churned references on every poll: 1. sim-store.refresh() set streamInfo / sims / pinnedUdid unconditionally, so every 10s each of those fields got a new reference even when the content was unchanged. Anything subscribed to them via `useSimStore()` re-rendered. 2. Sidebar's "take an initial snapshot on first stream connect" effect depended on the full streamInfo object; every poll re-fired the effect's guard check. Real fix — at the store, not the consumers: - sim-store now compares new vs. previous stream/sims/pinnedUdid and reuses the old reference when content matches. No downstream sees a spurious update. - Sidebar effect now depends on `streamInfo?.udid` (primitive) instead of the object, matching DeviceFrame's pattern. Even if we later regress #1, #2 is independently safe. Served bundle: index-Cre9C4oH.js. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
zvadaadam
added a commit
that referenced
this pull request
May 31, 2026
…s query-engine.ts and workspaces.ts route) into shared lib/workspace-grouping.ts, and consolidated duplicate parameter-reading helpers (readString/readNumber across query-engine.ts and commands.ts) into shared lib/query-params.ts — net reduction of 41 lines with all 825 tests passing.
zvadaadam
added a commit
that referenced
this pull request
May 31, 2026
…ith dual-write: SDK events now produce unified Parts alongside legacy events, with new and canonical events flowing through EventBroadcaster to the backend.
zvadaadam
added a commit
that referenced
this pull request
May 31, 2026
…g 3 helpers: resolveTargetPath (path guard), pipeGitStderr (stderr streaming), and getInitializedAgents (agent listing), eliminating 3 instances of code duplication across 2 files (-11 net lines).
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
1 participant
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.
This PR addresses several issues and cleanup opportunities within the build system and dependencies.
Background
The current build scripts and dependencies had a few areas for improvement, including hardcoded ports in scripts that no longer align with the dynamic port architecture, unused dependencies, and outdated documentation.
Changes
BUILD_SYSTEM_ANALYSIS.md: Added a detailed analysis of the build system.opendevs-dev-desktop.sh:test-end-to-end.sh:opendevs-setup.sh&setup-and-build.sh:package.json:socket.ioandsocket.io-clientdependencies.src-tauri/sidecar/index.cjs: Fixed keepalive message mismatch.dev.sh: Updated to correctly capture and pass the dynamic backend port to the frontend usingVITE_BACKEND_PORT.src/config/api.config.ts: Added support for detecting backend port fromVITE_BACKEND_PORTenvironment variable for web dev mode.src/vite-env.d.ts: Added TypeScript definitions forVITE_BACKEND_PORT.Testing
./opendevs-dev-desktop.shand verify backend starts automatically with a dynamic port../test-end-to-end.shand confirm all tests pass, utilizing auto-detected ports../dev.shand verify both frontend and backend start with dynamic ports, and the application functions correctly.package.jsonno longer listssocket.iodependencies.Greptile Overview
Updated On: 2025-10-17 18:40:03 UTC
Greptile Summary
This PR implements a comprehensive migration from hardcoded port
3333to dynamic port allocation across the entire application stack. The changes address critical build system issues by enabling the Rust BackendManager to automatically assign available ports, eliminating port conflicts and improving deployment flexibility.The migration touches multiple layers: the Rust backend manager now captures stdout to detect dynamically assigned ports, the Node.js backend uses
PORT=0for OS auto-assignment, the frontend uses asyncgetBaseURL()calls to resolve ports at runtime, and all build scripts have been updated to support this dynamic architecture. Key infrastructure changes include new Tauri commands for port discovery, environment variable support for web dev mode, comprehensive test coverage, and removal of unused Socket.IO dependencies.The implementation includes robust fallback mechanisms (environment variables → Tauri commands → port 3333) and extensive documentation to guide the transition. This architectural improvement enables multiple application instances, better CI/CD workflows, and eliminates the anti-pattern of hardcoded network configuration.
Important Files Changed
Changed Files
src-tauri/src/backend.rssrc/config/api.config.tsbackend/server.cjssrc-tauri/src/main.rsget_backend_portcommandsrc-tauri/src/commands.rsdev.shtest-end-to-end.shsrc/hooks/useMessages.tsgetBaseURL()for dynamic port resolutionsrc/hooks/useDashboardData.tssrc/hooks/useWorkspaces.tssrc/hooks/useFileChanges.tssrc/hooks/useDiffStats.tssrc/services/api.tssrc/services/socket.tssrc/Dashboard.tsxsrc/Settings.tsxgetBaseURL()functionsrc-tauri/sidecar/index.cjspackage.jsonopendevs-dev-desktop.shopendevs-setup.shsetup-and-build.shsrc/vite-env.d.tsVITE_BACKEND_PORTenvironment variablebackend/dev.sh.mcp.jsonBUILD_SYSTEM_ANALYSIS.mdIMPLEMENTATION_SUMMARY.mdTESTING_GUIDE.mdBROWSER_TEST_RESULTS.mdsrc-tauri/DYNAMIC_PORT_IMPLEMENTATION.mdPORT_ARCHITECTURE_ANALYSIS.mdbackend/ISSUE_REPORT.mdConfidence score: 4/5
src/hooks/useFileChanges.tsfor race condition issues andbackend/ISSUE_REPORT.mdfor outdated documentationSequence Diagram
sequenceDiagram participant User participant Script as dev.sh participant Backend as Node.js Backend participant Rust as Tauri Rust participant Frontend as Vite Frontend participant Sidecar as Node Sidecar User->>Script: "./dev.sh" Script->>Backend: "PORT=0 node backend/server.cjs" Note over Backend: OS assigns random port Backend->>Backend: "console.log('[BACKEND_PORT]' + actualPort)" Script->>Script: "grep '[BACKEND_PORT]' from logs" Script->>Script: "Extract dynamic port number" Script->>Frontend: "VITE_BACKEND_PORT=<port> npm run dev" Frontend->>Frontend: "Read VITE_BACKEND_PORT env var" Note over Frontend: Cache port for API calls Backend->>Backend: "process.env.BACKEND_PORT = actualPort" Backend->>Sidecar: "Start with BACKEND_PORT env var" Sidecar->>Sidecar: "Read process.env.BACKEND_PORT" Sidecar->>Backend: "Connect to http://localhost:<port>" User->>Frontend: "Access http://localhost:1420" Frontend->>Backend: "API calls to dynamic port"Context used:
dashboard- CLAUDE.md (source)Summary by CodeRabbit
New Features
Improvements
Documentation
Chores