feat(desktop): replace regex-based port detection with process-based scanning#610
Conversation
…scanning - Add pidtree dependency for cross-platform process tree traversal - Create port-scanner.ts with lsof (macOS/Linux) and netstat (Windows) support - Create port-hints.ts for lightweight output hint detection to trigger immediate scans - Rewrite port-manager.ts with process-based architecture: - Periodic scanning every 2.5s of all registered terminal sessions - Immediate scanning triggered by output hints (localhost:PORT, etc.) - Process tree traversal to find all child processes listening on ports - Update DetectedPort type with pid, processName, and address fields - Update UI tooltip to show process name instead of context line This fixes port detection for TUI apps like turborepo that use cursor positioning instead of linear output, making regex parsing unreliable.
📝 WalkthroughWalkthroughRefactors terminal port detection from line-based regex and per-port probes to a session-driven, process-tree scanning system with periodic and hint-triggered scans; adds cross-platform port-scanning utilities and tests, introduces pidtree dependency, updates session/manager integration, types, and UI to include PID/process name and address. Changes
Sequence Diagram(s)sequenceDiagram
participant Session as TerminalSession
participant Manager as TerminalManager
participant PM as PortManager
participant Scanner as PortScanner
participant UI as Renderer/UI
Note over Session,Manager: Session lifecycle
Session->>Manager: createSession()
Manager->>PM: registerSession(session, workspaceId)
Note right of Session: terminal output stream
Session->>PM: checkOutputForHint(data, paneId)
alt hint detected
PM->>PM: debounce & schedule scan(paneId)
PM->>Scanner: getProcessTree(session.pid)
Scanner-->>PM: pid[]
PM->>Scanner: getListeningPortsForPids(pid[])
Scanner-->>PM: PortInfo[]
PM->>PM: updatePortsForPane(paneId, PortInfo[])
PM-->>UI: emit port:add / port:remove events
UI->>UI: update PortsList (show processName, pid)
end
Session->>Manager: onExit()
Manager->>PM: unregisterSession(paneId)
PM->>PM: remove ports for paneId & emit port:remove
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI Agents
In @apps/desktop/src/main/lib/terminal/port-hints.ts:
- Around line 23-29: The framework regexes in the framework-specific patterns
array in port-hints.ts (the entries /vite/i, /express/i, /fastify/i) are too
broad and cause false positives; tighten them to require startup/context words
(e.g., require "dev", "server", "listening", "started", or similar) or combine
them with the existing startup message patterns above so the match only triggers
on actual server start lines—update the /vite/i, /express/i, and /fastify/i
entries to context-aware patterns (or add word-boundary/anchoring) so only
startup messages are matched.
In @apps/desktop/src/main/lib/terminal/port-manager.ts:
- Around line 69-72: The setTimeout callback currently calls
this.scanPane(paneId).catch(() => {}) which silently swallows errors; change it
to catch the error and log it with context (include paneId and that it occurred
during a hint scan) instead of an empty handler—update the promise error handler
on the this.scanPane(paneId) call so it logs the error via the project's logger
(or console.error) and still prevents unhandled rejections while referencing
pendingHintScans, scanPane, paneId and HINT_SCAN_DELAY_MS.
In @apps/desktop/src/main/lib/terminal/port-scanner.ts:
- Around line 73-94: The lsof parsing incorrectly uses columns[columns.length -
1] (which is "(LISTEN)") for the NAME field; change the code that assigns name
to use columns[columns.length - 2] (and guard for columns.length < 2) so you
extract the address:port token (e.g., "*:3000") before running the existing
regex and pushing into ports; keep the rest of the parsing (regex match, port
validation, ports.push with address mapping) unchanged.
🧹 Nitpick comments (6)
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/PortsList/PortsList.tsx (1)
202-202: Inconsistent spacing: use parent gap instead of per-badge margin.The parent container at line 152 uses
gap-1for horizontal spacing between badges, but addingmb-2here creates inconsistent spacing (1 unit horizontal vs 2 units vertical). Consider removingmb-2and adjusting the parent'sgap-1to handle both dimensions, or usegap-x-1 gap-y-2on the parent for consistent control.🔎 Proposed fix for consistent spacing
Remove
mb-2from the badge:<div - className={`group relative inline-flex items-center gap-1 rounded-md text-xs font-mono transition-colors mb-2 ${ + className={`group relative inline-flex items-center gap-1 rounded-md text-xs font-mono transition-colors ${ isCurrentWorkspaceAnd update the parent container at line 152:
- <div className="flex flex-wrap gap-1 px-3"> + <div className="flex flex-wrap gap-x-1 gap-y-2 px-3">apps/desktop/src/main/lib/terminal/port-hints.ts (1)
36-41: Extract magic number to named constant.The threshold value
10at line 38 should be extracted to a named constant at the module top for clarity and maintainability. As per coding guidelines, magic numbers should be extracted to named constants.🔎 Proposed fix
+/** Minimum data length to check for port hints - shorter output unlikely to contain port info */ +const MIN_HINT_DATA_LENGTH = 10; + /** * Check if terminal output contains hints that a port may have been opened. * This is a lightweight check - false positives are acceptable since we verify * with actual process scanning. */ export function containsPortHint(data: string): boolean { // Quick length check - very short output unlikely to contain port info - if (data.length < 10) return false; + if (data.length < MIN_HINT_DATA_LENGTH) return false; return HINT_PATTERNS.some((pattern) => pattern.test(data)); }apps/desktop/src/main/lib/terminal/port-scanner.ts (2)
46-59: Command injection risk with PID interpolation.While PIDs from
pidtreeshould be numbers, directly interpolating them into a shell command string could be risky if the input is ever compromised. Consider usingexecFilewith an array of arguments instead ofexecSyncwith shell interpolation for better security and to avoid shell parsing overhead.Additionally,
execSyncblocks the Node.js event loop during execution. For potentially long-running scans with many PIDs, this could impact UI responsiveness.🔎 Suggested safer approach using execFileSync
-import { execSync } from "node:child_process"; +import { execFileSync } from "node:child_process";function getListeningPortsLsof(pids: number[]): PortInfo[] { try { const pidArg = pids.join(","); - // -p: filter by PIDs - // -iTCP: only TCP connections - // -sTCP:LISTEN: only listening sockets - // -P: don't convert port numbers to names - // -n: don't resolve hostnames - // -F: output in parseable format (pid, command, name fields) - const output = execSync( - `lsof -p ${pidArg} -iTCP -sTCP:LISTEN -P -n 2>/dev/null || true`, - { encoding: "utf-8", maxBuffer: 10 * 1024 * 1024 }, - ); + const output = execFileSync( + "lsof", + ["-p", pidArg, "-iTCP", "-sTCP:LISTEN", "-P", "-n"], + { encoding: "utf-8", maxBuffer: 10 * 1024 * 1024, stdio: ["pipe", "pipe", "ignore"] }, + );
162-187: Good fallback pattern for deprecated wmic.The PowerShell fallback handles the wmic deprecation well. However, the same command injection concern applies here - consider validating PIDs are positive integers before interpolation.
🔎 Optional: Add PID validation
function getProcessNameWindows(pid: number): string { + // Validate PID is a positive integer + if (!Number.isInteger(pid) || pid <= 0) { + return "unknown"; + } + try { const output = execSync(apps/desktop/src/main/lib/terminal/port-manager.ts (2)
29-32: Consider lazy initialization of periodic scan.The periodic scan starts immediately in the constructor, even when no sessions are registered. This means the scan interval runs continuously from app startup. Consider starting the scan only when the first session is registered.
🔎 Suggested lazy start approach
constructor() { super(); - this.startPeriodicScan(); + // Scan will start when first session is registered } /** * Register a terminal session for port scanning */ registerSession(session: TerminalSession, workspaceId: string): void { this.sessions.set(session.paneId, { session, workspaceId }); + + // Start scanning if this is the first session + if (this.sessions.size === 1 && !this.scanInterval) { + this.startPeriodicScan(); + } }
146-158: Consider parallelizing process tree fetches.The sequential
await getProcessTree(pid)for each session could be slow with many terminal sessions. Consider usingPromise.allto fetch process trees in parallel.🔎 Suggested parallel approach
- for (const [paneId, { session, workspaceId }] of this.sessions) { - if (!session.isAlive) continue; - - try { - const pid = session.pty.pid; - const pids = await getProcessTree(pid); - if (pids.length > 0) { - panePortMap.set(paneId, { workspaceId, pids }); - } - } catch { - // Session may have exited - } - } + const sessionEntries = Array.from(this.sessions.entries()) + .filter(([, { session }]) => session.isAlive); + + const results = await Promise.all( + sessionEntries.map(async ([paneId, { session, workspaceId }]) => { + try { + const pid = session.pty.pid; + const pids = await getProcessTree(pid); + return pids.length > 0 ? { paneId, workspaceId, pids } : null; + } catch { + return null; + } + }) + ); + + for (const result of results) { + if (result) { + panePortMap.set(result.paneId, { workspaceId: result.workspaceId, pids: result.pids }); + } + }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
apps/desktop/package.jsonapps/desktop/src/main/lib/terminal/manager.tsapps/desktop/src/main/lib/terminal/port-hints.tsapps/desktop/src/main/lib/terminal/port-manager.tsapps/desktop/src/main/lib/terminal/port-scanner.tsapps/desktop/src/main/lib/terminal/session.tsapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/PortsList/PortsList.tsxapps/desktop/src/shared/types/ports.ts
🧰 Additional context used
📓 Path-based instructions (6)
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: For Electron interprocess communication, ALWAYS use tRPC as defined insrc/lib/trpc
Use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from@trpc/server/observableinstead of async generators, as the library explicitly checksisObservable(result)and throws an error otherwise
Files:
apps/desktop/src/main/lib/terminal/port-hints.tsapps/desktop/src/shared/types/ports.tsapps/desktop/src/main/lib/terminal/session.tsapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/PortsList/PortsList.tsxapps/desktop/src/main/lib/terminal/port-manager.tsapps/desktop/src/main/lib/terminal/manager.tsapps/desktop/src/main/lib/terminal/port-scanner.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use object parameters for functions with 2+ parameters instead of positional arguments
Functions with 2+ parameters should accept a single params object with named properties for self-documentation and extensibility
Use prefixed console logging with context pattern: [domain/operation] message
Extract magic numbers and hardcoded values to named constants at module top
Use lookup objects/maps instead of repeated if (type === ...) conditionals
Avoid usinganytype - maintain type safety in TypeScript code
Never swallow errors silently - at minimum log them with context
Import from concrete files directly when possible - avoid barrel file abuse that creates circular dependencies
Avoid deep nesting (4+ levels) - use early returns, extract functions, and invert conditions
Use named properties in options objects instead of boolean parameters to avoid boolean blindness
Files:
apps/desktop/src/main/lib/terminal/port-hints.tsapps/desktop/src/shared/types/ports.tsapps/desktop/src/main/lib/terminal/session.tsapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/PortsList/PortsList.tsxapps/desktop/src/main/lib/terminal/port-manager.tsapps/desktop/src/main/lib/terminal/manager.tsapps/desktop/src/main/lib/terminal/port-scanner.ts
apps/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Drizzle ORM for all database operations - never use raw SQL
Files:
apps/desktop/src/main/lib/terminal/port-hints.tsapps/desktop/src/shared/types/ports.tsapps/desktop/src/main/lib/terminal/session.tsapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/PortsList/PortsList.tsxapps/desktop/src/main/lib/terminal/port-manager.tsapps/desktop/src/main/lib/terminal/manager.tsapps/desktop/src/main/lib/terminal/port-scanner.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Biome for formatting and linting - run at root level with
bun run lint:fixorbiome check --write
Files:
apps/desktop/src/main/lib/terminal/port-hints.tsapps/desktop/src/shared/types/ports.tsapps/desktop/src/main/lib/terminal/session.tsapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/PortsList/PortsList.tsxapps/desktop/src/main/lib/terminal/port-manager.tsapps/desktop/src/main/lib/terminal/manager.tsapps/desktop/src/main/lib/terminal/port-scanner.ts
apps/desktop/src/renderer/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never import Node.js modules (fs, path, os, net) in renderer process or shared code - they are externalized for browser compatibility
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/PortsList/PortsList.tsx
**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
One component per file - do not create multi-component files
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/PortsList/PortsList.tsx
🧠 Learnings (1)
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to apps/desktop/src/renderer/**/*.{ts,tsx} : Never import Node.js modules (fs, path, os, net) in renderer process or shared code - they are externalized for browser compatibility
Applied to files:
apps/desktop/src/main/lib/terminal/port-scanner.ts
🧬 Code graph analysis (2)
apps/desktop/src/main/lib/terminal/session.ts (1)
apps/desktop/src/main/lib/terminal/port-manager.ts (1)
portManager(293-293)
apps/desktop/src/main/lib/terminal/port-manager.ts (4)
apps/desktop/src/main/lib/terminal/types.ts (1)
TerminalSession(5-22)apps/desktop/src/shared/types/ports.ts (1)
DetectedPort(1-9)apps/desktop/src/main/lib/terminal/port-hints.ts (1)
containsPortHint(36-41)apps/desktop/src/main/lib/terminal/port-scanner.ts (2)
getProcessTree(15-22)getListeningPortsForPids(28-41)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Deploy Admin
- GitHub Check: Deploy Docs
- GitHub Check: Deploy API
- GitHub Check: Deploy Marketing
- GitHub Check: Deploy Web
- GitHub Check: Build
🔇 Additional comments (13)
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/PortsList/PortsList.tsx (1)
228-230: LGTM!The tooltip correctly displays the new process-based port metadata (
processNameandpid), which aligns with the refactoredDetectedPorttype and provides more actionable information than the previouscontextLine.apps/desktop/src/shared/types/ports.ts (1)
1-9: LGTM!The updated
DetectedPortinterface correctly reflects the shift to process-based detection. The new fields (pid,processName,address) provide richer metadata, and replacingcontextLinewith structured data is a solid improvement.apps/desktop/src/main/lib/terminal/session.ts (2)
99-100: LGTM!Clear comment explaining the architectural shift from scrollback-based port scanning to process-based detection via PortManager.
167-168: LGTM!The integration with
portManager.checkOutputForHintcorrectly replaces inline port scanning with hint-based triggering, aligning with the process-based detection architecture.apps/desktop/package.json (1)
95-95: pidtree package version 0.6.0 is valid and secure.The version is available on npm as the latest release (published 2022-06-05) and has no known security vulnerabilities reported.
apps/desktop/src/main/lib/terminal/manager.ts (2)
95-97: LGTM - Port manager registration properly placed.The session is registered with the port manager after it's created and stored in the sessions map, ensuring the session is fully initialized before port scanning begins.
148-149: LGTM - Clean unregistration on session exit.Properly unregisters the session before emitting the exit event, ensuring ports are cleaned up synchronously when the terminal exits.
apps/desktop/src/main/lib/terminal/port-scanner.ts (2)
105-157: Windows netstat parsing looks correct.The column extraction properly handles the netstat output format. The process name caching is a good optimization to avoid repeated wmic/PowerShell calls.
192-211: LGTM - Cross-platform process name lookup.The implementation correctly delegates to the appropriate platform-specific method. The comment about macOS
commtruncation is helpful context.apps/desktop/src/main/lib/terminal/port-manager.ts (4)
112-129: LGTM - Scan implementation with proper error handling.The try/catch properly handles the potential race condition where a session could exit between the
isAlivecheck and accessingpty.pid. The error logging provides good context for debugging.
181-244: LGTM - Port update logic handles all cases correctly.The implementation properly handles:
- New port detection with
port:addevent- Port updates (when PID/process changes) via
port:remove+port:add- Stale port cleanup for ports no longer listening
The
seenKeystracking ensures accurate diff detection.
253-290: LGTM - Helper methods are well implemented.The port removal, querying, and force scan methods are straightforward and correctly implemented.
7-15: Good use of named constants for configuration.The constants
SCAN_INTERVAL_MS,HINT_SCAN_DELAY_MS, andIGNORED_PORTSare well-named and appropriately placed at module top, following the coding guidelines for extracting magic numbers.
| // Framework-specific patterns | ||
| /next\.?js/i, // Next.js | ||
| /vite/i, // Vite | ||
| /webpack.*compiled/i, // Webpack dev server | ||
| /express/i, // Express | ||
| /fastify/i, // Fastify | ||
| ]; |
There was a problem hiding this comment.
Overly broad framework patterns may trigger excessive false-positive scans.
While the comment notes that false positives are acceptable, patterns like /vite/i, /express/i, and /fastify/i match the framework name anywhere in output—including error messages, file paths, or unrelated text. This could trigger unnecessary process scans frequently, impacting performance.
Consider making these patterns more specific by requiring context (e.g., /vite.*(?:dev|server)/i, /express.*(?:listening|server)/i) or combining them with the startup message patterns above.
🔎 Proposed more specific patterns
// Framework-specific patterns
- /next\.?js/i, // Next.js
- /vite/i, // Vite
+ /next(?:\.js)?\s+(?:dev|server|ready)/i, // Next.js
+ /vite\s+(?:dev|server|preview)/i, // Vite
/webpack.*compiled/i, // Webpack dev server
- /express/i, // Express
- /fastify/i, // Fastify
+ /express.*(?:listening|started|server)/i, // Express
+ /fastify.*(?:listening|started|server)/i, // Fastify📝 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.
| // Framework-specific patterns | |
| /next\.?js/i, // Next.js | |
| /vite/i, // Vite | |
| /webpack.*compiled/i, // Webpack dev server | |
| /express/i, // Express | |
| /fastify/i, // Fastify | |
| ]; | |
| // Framework-specific patterns | |
| /next(?:\.js)?\s+(?:dev|server|ready)/i, // Next.js | |
| /vite\s+(?:dev|server|preview)/i, // Vite | |
| /webpack.*compiled/i, // Webpack dev server | |
| /express.*(?:listening|started|server)/i, // Express | |
| /fastify.*(?:listening|started|server)/i, // Fastify | |
| ]; |
🤖 Prompt for AI Agents
In @apps/desktop/src/main/lib/terminal/port-hints.ts around lines 23-29, The
framework regexes in the framework-specific patterns array in port-hints.ts (the
entries /vite/i, /express/i, /fastify/i) are too broad and cause false
positives; tighten them to require startup/context words (e.g., require "dev",
"server", "listening", "started", or similar) or combine them with the existing
startup message patterns above so the match only triggers on actual server start
lines—update the /vite/i, /express/i, and /fastify/i entries to context-aware
patterns (or add word-boundary/anchoring) so only startup messages are matched.
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In @apps/desktop/src/main/lib/terminal/port-manager.ts:
- Around line 145-153: The catch block swallowing errors around the process-tree
scan should log the error with context instead of being empty; update the
try/catch around getProcessTree(pid) (where pid is taken from session.pty.pid)
to catch (err) and call the module logger (or processLogger) to record a clear
message including paneId, workspaceId and the pid and the error object so
unexpected failures are visible while preserving the existing comment about
sessions exiting.
- Around line 68-71: Replace the silent .catch(() => {}) on the
this.scanPane(paneId) promise with an error-logging handler that records the
error and context instead of swallowing it; specifically, in the timeout
callback where timeout is set and this.pendingHintScans.delete(paneId) is
called, change the catch to something like .catch(err => { /* use existing
logger (e.g. processLogger.error) or console.error */ }) and include paneId and
that the failure came from a scheduled hint scan (HINT_SCAN_DELAY_MS) in the log
message so the error is observable while preserving non-throwing behavior.
🧹 Nitpick comments (4)
apps/desktop/src/main/lib/terminal/port-scanner.test.ts (1)
14-76: Consider reducing code duplication between test and implementation.The
PortInfointerface (lines 14-19) andparseLsofOutputfunction (lines 30-76) duplicate logic fromport-scanner.ts. If the parsing implementation changes, these tests won't catch regressions unless the test copy is manually kept in sync.Options to reduce duplication:
- Export
parseLsofOutputfromport-scanner.tsas a public utility and import it here- Test indirectly through the public
getListeningPortsForPids()API with mockedexecSync- Keep as-is if isolating parsing logic for unit tests is preferred, but add a comment noting the duplication risk
The test coverage itself is excellent—just flag the maintenance risk.
apps/desktop/src/main/lib/terminal/port-scanner.ts (1)
15-22: Consider logging errors in catch blocks for observability.The empty catch block returns an empty array when
pidtreefails, which is reasonable when a process has exited. However, per coding guidelines, even expected failures benefit from logging to aid debugging. Consider logging at debug level:} catch (error) { // Process may have exited - this is expected console.debug(`[PortScanner] getProcessTree failed for pid ${pid}:`, error); return []; }The same pattern could be applied to lines 103, 157, 176-184, and 208.
apps/desktop/src/main/lib/terminal/port-manager.ts (2)
37-39: Use object parameters for functions with 2+ arguments.Per coding guidelines, functions with 2+ parameters should accept a single params object for self-documentation and extensibility.
🔎 Proposed refactor
/** * Register a terminal session for port scanning */ - registerSession(session: TerminalSession, workspaceId: string): void { - this.sessions.set(session.paneId, { session, workspaceId }); + registerSession({ session, workspaceId }: { session: TerminalSession; workspaceId: string }): void { + this.sessions.set(session.paneId, { session, workspaceId }); }Based on coding guidelines.
175-184: Use object parameters for functions with 3+ arguments.This private method has 3 parameters and would benefit from an object parameter for clarity and extensibility, per coding guidelines.
🔎 Proposed refactor
/** * Update ports for a specific pane, emitting add/remove events as needed */ private updatePortsForPane( - paneId: string, - workspaceId: string, - portInfos: Array<{ + { paneId, workspaceId, portInfos }: { + paneId: string; + workspaceId: string; + portInfos: Array<{ port: number; pid: number; address: string; processName: string; - }>, + }>}, ): void {Then update the call sites at lines 123 and 158.
Based on coding guidelines.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/desktop/src/lib/trpc/routers/projects/projects.tsapps/desktop/src/main/lib/terminal/manager.tsapps/desktop/src/main/lib/terminal/port-hints.tsapps/desktop/src/main/lib/terminal/port-manager.tsapps/desktop/src/main/lib/terminal/port-scanner.test.tsapps/desktop/src/main/lib/terminal/port-scanner.ts
💤 Files with no reviewable changes (1)
- apps/desktop/src/lib/trpc/routers/projects/projects.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/src/main/lib/terminal/manager.ts
- apps/desktop/src/main/lib/terminal/port-hints.ts
🧰 Additional context used
📓 Path-based instructions (5)
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: For Electron interprocess communication, ALWAYS use tRPC as defined insrc/lib/trpc
Use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from@trpc/server/observableinstead of async generators, as the library explicitly checksisObservable(result)and throws an error otherwise
Files:
apps/desktop/src/main/lib/terminal/port-scanner.test.tsapps/desktop/src/main/lib/terminal/port-manager.tsapps/desktop/src/main/lib/terminal/port-scanner.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use object parameters for functions with 2+ parameters instead of positional arguments
Functions with 2+ parameters should accept a single params object with named properties for self-documentation and extensibility
Use prefixed console logging with context pattern: [domain/operation] message
Extract magic numbers and hardcoded values to named constants at module top
Use lookup objects/maps instead of repeated if (type === ...) conditionals
Avoid usinganytype - maintain type safety in TypeScript code
Never swallow errors silently - at minimum log them with context
Import from concrete files directly when possible - avoid barrel file abuse that creates circular dependencies
Avoid deep nesting (4+ levels) - use early returns, extract functions, and invert conditions
Use named properties in options objects instead of boolean parameters to avoid boolean blindness
Files:
apps/desktop/src/main/lib/terminal/port-scanner.test.tsapps/desktop/src/main/lib/terminal/port-manager.tsapps/desktop/src/main/lib/terminal/port-scanner.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Co-locate tests with implementation files using .test.ts or .test.tsx suffix
Files:
apps/desktop/src/main/lib/terminal/port-scanner.test.ts
apps/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Drizzle ORM for all database operations - never use raw SQL
Files:
apps/desktop/src/main/lib/terminal/port-scanner.test.tsapps/desktop/src/main/lib/terminal/port-manager.tsapps/desktop/src/main/lib/terminal/port-scanner.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Biome for formatting and linting - run at root level with
bun run lint:fixorbiome check --write
Files:
apps/desktop/src/main/lib/terminal/port-scanner.test.tsapps/desktop/src/main/lib/terminal/port-manager.tsapps/desktop/src/main/lib/terminal/port-scanner.ts
🧠 Learnings (2)
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to **/*.{ts,tsx} : Never swallow errors silently - at minimum log them with context
Applied to files:
apps/desktop/src/main/lib/terminal/port-manager.ts
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to apps/desktop/src/renderer/**/*.{ts,tsx} : Never import Node.js modules (fs, path, os, net) in renderer process or shared code - they are externalized for browser compatibility
Applied to files:
apps/desktop/src/main/lib/terminal/port-scanner.ts
🧬 Code graph analysis (1)
apps/desktop/src/main/lib/terminal/port-scanner.test.ts (1)
apps/desktop/src/main/lib/terminal/port-scanner.ts (1)
PortInfo(5-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Deploy Docs
- GitHub Check: Deploy API
- GitHub Check: Deploy Marketing
- GitHub Check: Deploy Admin
- GitHub Check: Deploy Web
- GitHub Check: Build
🔇 Additional comments (1)
apps/desktop/src/main/lib/terminal/port-scanner.ts (1)
82-82: Previously reported parsing bug has been fixed.The code now correctly uses
columns[columns.length - 2]to extract the NAME column (e.g.,*:3000) before the(LISTEN)state marker. The tests inport-scanner.test.tsvalidate this fix extensively.
| const timeout = setTimeout(() => { | ||
| this.pendingHintScans.delete(paneId); | ||
| this.scanPane(paneId).catch(() => {}); | ||
| }, HINT_SCAN_DELAY_MS); |
There was a problem hiding this comment.
Avoid silently swallowing errors.
The empty .catch(() => {}) swallows any scan errors without logging. Per coding guidelines, errors should at minimum be logged with context.
🔎 Proposed fix
const timeout = setTimeout(() => {
this.pendingHintScans.delete(paneId);
- this.scanPane(paneId).catch(() => {});
+ this.scanPane(paneId).catch((error) => {
+ console.error(`[PortManager] Hint-triggered scan failed for ${paneId}:`, error);
+ });
}, HINT_SCAN_DELAY_MS);📝 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.
| const timeout = setTimeout(() => { | |
| this.pendingHintScans.delete(paneId); | |
| this.scanPane(paneId).catch(() => {}); | |
| }, HINT_SCAN_DELAY_MS); | |
| const timeout = setTimeout(() => { | |
| this.pendingHintScans.delete(paneId); | |
| this.scanPane(paneId).catch((error) => { | |
| console.error(`[PortManager] Hint-triggered scan failed for ${paneId}:`, error); | |
| }); | |
| }, HINT_SCAN_DELAY_MS); |
🤖 Prompt for AI Agents
In @apps/desktop/src/main/lib/terminal/port-manager.ts around lines 68 - 71,
Replace the silent .catch(() => {}) on the this.scanPane(paneId) promise with an
error-logging handler that records the error and context instead of swallowing
it; specifically, in the timeout callback where timeout is set and
this.pendingHintScans.delete(paneId) is called, change the catch to something
like .catch(err => { /* use existing logger (e.g. processLogger.error) or
console.error */ }) and include paneId and that the failure came from a
scheduled hint scan (HINT_SCAN_DELAY_MS) in the log message so the error is
observable while preserving non-throwing behavior.
| try { | ||
| const pid = session.pty.pid; | ||
| const pids = await getProcessTree(pid); | ||
| if (pids.length > 0) { | ||
| panePortMap.set(paneId, { workspaceId, pids }); | ||
| } | ||
| } catch { | ||
| // Session may have exited | ||
| } |
There was a problem hiding this comment.
Log errors in catch block for session scan failures.
The empty catch block at lines 151-152 silently ignores errors when getting the process tree for a session. While the comment indicates this is expected ("Session may have exited"), logging would help with debugging unexpected failures. Per coding guidelines, errors should not be swallowed silently.
🔎 Proposed fix
try {
const pid = session.pty.pid;
const pids = await getProcessTree(pid);
if (pids.length > 0) {
panePortMap.set(paneId, { workspaceId, pids });
}
- } catch {
- // Session may have exited
+ } catch (error) {
+ // Session may have exited - log for debugging
+ console.debug(`[PortManager] Failed to get process tree for ${paneId}:`, error);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| const pid = session.pty.pid; | |
| const pids = await getProcessTree(pid); | |
| if (pids.length > 0) { | |
| panePortMap.set(paneId, { workspaceId, pids }); | |
| } | |
| } catch { | |
| // Session may have exited | |
| } | |
| try { | |
| const pid = session.pty.pid; | |
| const pids = await getProcessTree(pid); | |
| if (pids.length > 0) { | |
| panePortMap.set(paneId, { workspaceId, pids }); | |
| } | |
| } catch (error) { | |
| // Session may have exited - log for debugging | |
| console.debug(`[PortManager] Failed to get process tree for ${paneId}:`, error); | |
| } |
🤖 Prompt for AI Agents
In @apps/desktop/src/main/lib/terminal/port-manager.ts around lines 145 - 153,
The catch block swallowing errors around the process-tree scan should log the
error with context instead of being empty; update the try/catch around
getProcessTree(pid) (where pid is taken from session.pty.pid) to catch (err) and
call the module logger (or processLogger) to record a clear message including
paneId, workspaceId and the pid and the error object so unexpected failures are
visible while preserving the existing comment about sessions exiting.
1 participant
Summary
lsof(macOS/Linux) andnetstat(Windows)Changes
pidtreedependency for cross-platform process tree traversalport-scanner.tswith platform-specific utilities for finding listening portsport-hints.tsfor lightweight output hint detection to trigger immediate scansport-manager.tswith new architecture:DetectedPorttype withpid,processName, andaddressfieldsSummary by CodeRabbit
New Features
Improvements
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.