Skip to content

Conversation

@sreya
Copy link
Contributor

@sreya sreya commented Nov 12, 2025

No description provided.

sreya added 30 commits November 13, 2025 07:30
- Downgrade electron ^38.2.1 → ^31.7.7 (Node 22→20, C++20→C++17)
- Add node-pty ^1.0.0 to dependencies
- node-pty@1.0.0 + nan@2.17.0 requires C++17, incompatible with Electron 38's C++20
- Electron 31 is last version before C++20 requirement
- Verified node-pty builds successfully and spawns PTY processes

_Generated with `cmux`_
- Add node-pty dependency for local PTY sessions
- Link ghostty-wasm library for terminal rendering
- Implement PTYService for managing terminal sessions (local and SSH)
- Implement TerminalServer for WebSocket-based terminal I/O
- Add IPC handlers for terminal create/close/resize operations
- Support SSH terminals via runtime.exec() with PTY allocation (-t flag)
- Add terminal types and IPC constants
- Start terminal server on IpcMain initialization

Phase 1 of 2 for embedded terminal feature.
Backend is complete and type-checks.
- Add terminal methods to IPCApi interface
- Implement terminal IPC in preload.ts
- Add terminal mock implementations for browser and storybook
- Import terminal types in ipc.ts

Prepares frontend for terminal component implementation.
- Implement useTerminalSession hook for WebSocket connection management
- Create TerminalView component with ghostty-wasm integration
- Integrate terminal toggle into AIView (Ctrl+` keybind)
- Add WASM asset support to vite.config
- Terminal appears at bottom of chat area (VS Code style)
- Per-workspace terminal visibility persistence

Frontend complete - embedded terminal now functional.
- Fix Infinity timeout issue causing SSH terminals to fail immediately
- Use 24-hour timeout instead of Infinity for SSH terminals
- Add CMUX_ALLOW_MULTIPLE_INSTANCES env var for development
- Fix WASM path for dev mode (/src/assets instead of /assets)
- Switch to node-pty-prebuilt-multiarch (local terminals need ABI 139)

SSH terminals now work. Local terminals require node-pty ABI 139 prebuilt.
- Fixed useTerminalSession dependency array to exclude sessionId
  - sessionId in deps caused infinite recreation loop
  - Now uses closure variable for cleanup instead
- Added comprehensive debug logging for terminal lifecycle
  - Tracks session creation, WebSocket events, and cleanup
  - Logs SSH command execution and stream state
  - Helps diagnose rapid exit issues
- Removed unused @ts-expect-error for ghostty-wasm import
- Added node-pty-prebuilt-multiarch (still lacks ABI 139 for Electron 38)

This should resolve the SSH terminal reconnection loop. Local terminals
still blocked on node-pty ABI 139 availability for Electron 38.

Generated with `cmux`
Moved node-pty require() inside the LocalRuntime branch so it only
loads when actually creating a local terminal. This prevents the app
from crashing on startup when node-pty binaries are missing.

Error message now clearly explains the ABI mismatch and suggests
using SSH workspaces as a workaround.

Generated with `cmux`
The log service is a Node.js backend service and cannot be imported
in renderer/frontend code. Changed useTerminalSession to use console
methods instead, which work in the browser context.

This fixes the blank screen issue caused by trying to import backend
services in the frontend.

Generated with `cmux`
Added debug logs to track terminal initialization steps:
- Component lifecycle (mount/unmount)
- Terminal instance creation
- FitAddon loading
- DOM mounting and fitting

This will help diagnose why the terminal window opens but shows no content.

Generated with `cmux`
Added logging to track WebSocket → Terminal data flow:
- WebSocket effect lifecycle (setup/teardown)
- Message receipt (type and data length)
- Terminal write operations

This will show if PTY output is reaching the terminal and being written.

Generated with `cmux`
The useEffect for WebSocket messages had wsRef in the dependency array,
but refs don't trigger effects. Changed to only depend on 'connected'
and capture ws/term refs at the start of the effect.

This should fix the issue where terminal window opens but shows no output.

Generated with `cmux`
Added terminalReady state to track when terminal is fully initialized.
This prevents the WebSocket message handler from being set up before
the terminal ref is available, which was causing the timing issue where
messages arrived but had nowhere to go.

Generated with `cmux`
Added automatic newline send after WebSocket handler setup to trigger
the shell to output its prompt. This helps diagnose if the issue is:
- No shell output (should see prompt after newline)
- WebSocket not receiving data (will see in logs)
- Terminal not rendering (will appear in DOM)

Generated with `cmux`
Changed log.debug to log.info for PTY SSH logs so they always appear
in the console. This will help us see what's happening with the SSH
connection and why no data is flowing.

Generated with `cmux`
Added error handling around runtime.exec() call to see if it's failing
silently. Also added logs before and after the exec call to track if
it's hanging or throwing an error.

Generated with `cmux`
The terminal server only knew which WebSocket belonged to which session
after receiving the first input message. This meant PTY output that
arrived before any input was sent had nowhere to go.

Added 'attach' message type that frontend sends immediately after
WebSocket connection to register the connection with the session.

This should finally make terminal output appear!

Generated with `cmux`
The command 'exec $SHELL' was exiting with code 127 (command not found)
because $SHELL variable wasn't being expanded properly in the SSH context.

Switched to 'bash -l' which starts a login bash shell, which is more
reliable and available on most SSH servers.

Generated with `cmux`
- Implement PTYService for managing terminal sessions (SSH + local)
- Add TerminalServer WebSocket bridge for terminal I/O
- Integrate ghostty-wasm for terminal rendering
- Add terminal IPC handlers and types
- Use 'script' command to create proper PTY for SSH sessions
- Handle both stdout and stderr (zsh sends prompt to stderr)
- Implement persistent stdin writer to avoid lock contention
- Add keyboard shortcuts (Cmd+T) and prevent focus stealing
- Fixed-size terminals (100x20) for SSH due to PTY limitations

Known limitations:
- SSH terminals cannot dynamically resize (fixed at creation)
- Local terminals blocked by node-pty ABI mismatch (Electron 38 needs ABI 139)
- Some input lag due to SSH round-trip

Generated with `cmux`
- vite-plugin-top-level-await uses Node.js Module API incompatible with Bun
- Vite 7+ handles top-level await natively with target: esnext
- Build now succeeds and produces dist/

_Generated with `cmux`_
- Bun's Module class doesn't implement Node.js Module API
- Plugin uses Module._resolveFilename which doesn't exist in Bun
- Patch to use direct require() first, fallback to Module API
- Run scripts/patch-vite-plugin.sh after bun install

_Generated with `cmux`_
- Main process is built as CommonJS (tsconfig.main.json)
- chalk v5+ is ESM-only, incompatible with require()
- Downgrade to chalk@4.1.2 which supports CommonJS

_Generated with `cmux`_
- Add '-l' flag to spawn login shell (loads .zshrc, .bash_profile, etc.)
- Explicitly set TERM=xterm-256color in env
- Ensures proper shell initialization for local terminals

_Generated with `cmux`_
- Set TERM_PROGRAM=cmux to identify embedded terminal
- Set DISABLE_AUTO_TITLE=true to prevent title escape sequences
- Set ZSH_AUTOSUGGEST_STRATEGY=history for simpler completions
- Reduces rendering issues with complex zsh plugins/prompts

User can detect cmux terminal in .zshrc with:
  if [[ $TERM_PROGRAM == 'cmux' ]]; then
    # Use simpler prompt/plugins
  fi

_Generated with `cmux`_
- Wait for terminal to fit before creating PTY session
- Use actual terminal dimensions instead of hardcoded 100x20
- Fixes tab completion rendering corruption (shell width matches display width)
- Stabilize terminalSize object references to prevent unnecessary re-renders
- Add sessionId to message handler deps to reconnect on toggle
- Fixes blank terminal when toggling visibility off and back on

Resolves terminal rendering issues identified in ghostty-wasm integration.
- Electron 31.7.7 → 38.6.0 (Node 20 → 22, C++17 → C++20)
- node-pty 1.0.0 → 1.1.0-beta39
- node-pty beta uses node-addon-api (N-API) instead of nan
- N-API is ABI-stable and C++20 compatible
- Successfully rebuilt node-pty native module (84KB)
- Binary builds for Electron 38's Node 22 ABI

Testing in progress to verify terminal functionality works correctly.
- Update import from ghostty-wasm to ghostty-web package
- Add ghostty-web as git dependency from github.com/coder/ghostty-web
- Add workspace path validation before spawning PTY
- Enhance error logging with shell, cwd, and environment details
- Ensure PATH is properly set in PTY environment

_Generated with `cmux`_
sreya added 21 commits November 14, 2025 16:16
Terminals now open in new browser windows/tabs when running in server mode:

1. Added stub handlers for TERMINAL_WINDOW_OPEN and TERMINAL_WINDOW_CLOSE
   in main-server.ts to prevent 404 errors

2. Updated browser API to call window.open() when opening a terminal,
   which creates a new browser window/tab pointing to /terminal.html

3. Removed debug logging from App.tsx

Browser mode terminals work the same as desktop:
- Click terminal button or use Ctrl+T (not Cmd+T, that opens browser tab)
- Opens /terminal.html?workspaceId=<id> in new window
- Terminal connects via WebSocket and works like desktop mode

Generated with `mux`
Terminal windows need the browser API to communicate with the backend
in server mode. Added dynamic import of browser/api.ts when window.api
is not already set up (i.e., when not running in Electron with preload).

Generated with `mux`
In browser mode, BrowserWindow is undefined and event.sender is null.
Updated TERMINAL_CREATE handler to use the mainWindow (mockWindow) when
event.sender is null, which broadcasts terminal events to all WebSocket
clients.

Generated with `mux`
In server mode, the entire event object is null (not just event.sender).
Use optional chaining to check event?.sender before accessing it.

Generated with `mux`
In server mode, HttpIpcMainAdapter only creates endpoints for handle()
methods, not on() methods. Changed TERMINAL_INPUT to use handle() so
it works in both Electron and browser modes.

Generated with `mux`
Added timestamp to window.open() name parameter so each Ctrl+T creates
a new terminal window instead of reusing the existing one.

Generated with `mux`
Removed console.log statements added during debugging.

Generated with `mux`
Added platform: 'browser' to browser API so components can detect when
running in browser vs Electron. Updated TitleBar to skip update checks
when platform === 'browser', eliminating the need for stub handlers in
main-server.ts.

Generated with `mux`
Changed nodemon to only watch main-server.js and run it directly, not
via main.js. This prevents the 'too many arguments' error since main.js
is the Electron entry point and doesn't accept server arguments.

Also increased delay from 500ms to 1000ms for consistency.

Generated with `mux`
Updated terminal-window.tsx to fetch workspace metadata and set
document.title to 'Terminal — project/workspace' instead of showing
the raw URL in browser tabs.

Falls back to workspace ID if metadata can't be fetched.

Generated with `mux`
Changed TERMINAL_WINDOW_OPEN handler to silently return when
terminalWindowManager is not available (server mode) instead of throwing
an error. In server mode, window.open() handles opening the terminal.

Generated with `mux`
Changed from non-existent workspace.getMetadata() to workspace.list()
and find the matching workspace by ID.

Generated with `mux`
Added 'electron-builder install-app-deps' as postinstall script to ensure
node-pty and other native dependencies are rebuilt for the correct Electron
version during CI builds on Linux and macOS.

Fixes 'bun rebuild' not found error in electron-builder.

Generated with `mux`
@sreya sreya requested a review from kylecarbs November 14, 2025 23:34
Copy link
Member

@kylecarbs kylecarbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approved from me but you should prob get ammar to review before merge

Comment on lines +6 to +7
// Remove 'server' from args since main-server doesn't expect it as a positional argument.
process.argv.splice(2, 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm confused by this i don't think this is needed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something about my changes upgraded commander to v14 which enforces stricter args parsing. Because main-server.ts isn't expecting args but we're passing it server it crashes out on 14

@sreya sreya enabled auto-merge November 15, 2025 18:15
@sreya sreya added this pull request to the merge queue Nov 15, 2025
Merged via the queue into main with commit b291885 Nov 15, 2025
18 of 19 checks passed
@sreya sreya deleted the local-pty branch November 15, 2025 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants