Codex web app architecture#8
Conversation
- Create apps/server with package.json, tsconfig, tsup config - Copy codexAppServerManager, providerManager, fixPath from desktop (no Electron deps) - Add WebSocket message protocol schemas (WsRequest, WsResponse, WsPush) to @acme/contracts - Define WS_METHODS and WS_CHANNELS constants for type-safe routing Co-authored-by: Theo Browne <t3dotgg@users.noreply.github.com>
- wsServer.ts: WebSocket server routes NativeApi methods, serves static files - Provider events pushed to all connected clients - SPA fallback for client-side routing - Server welcome message with cwd/projectName on connect - index.ts: CLI entry point - Auto-detects available port (default 3773) - Resolves renderer dist for static serving - Opens browser on startup via 'open' package - Graceful shutdown on SIGINT/SIGTERM Co-authored-by: Theo Browne <t3dotgg@users.noreply.github.com>
- wsTransport.ts: WebSocket client with request/response, push subscriptions, auto-reconnect with exponential backoff - wsNativeApi.ts: Full NativeApi implementation over WebSocket transport - All provider methods routed through WS_METHODS - Provider events streamed via WS_CHANNELS.providerEvent - Todos/terminal/agent stubbed (not needed for web mode) - dialogs.pickFolder returns null (text input fallback in UI) - session-logic.ts: readNativeApi() now auto-detects environment - Prefers Electron preload bridge if available (window.nativeApi) - Falls back to WebSocket transport for browser mode - onServerWelcome() hook for renderer to receive cwd on connect Co-authored-by: Theo Browne <t3dotgg@users.noreply.github.com>
- App.tsx: Add AutoProjectBootstrap that listens for server welcome and auto-creates project + thread from server's cwd - Sidebar: Conditionalize drag region (Electron only), hide Browse button in browser - ChatView: Conditionalize drag region, adjust padding for browser - DiffPanel: Same drag region treatment - index.html: Update title to 'CodeThing' - env.ts: isElectron detection utility Co-authored-by: Theo Browne <t3dotgg@users.noreply.github.com>
- Root package.json: Add dev, dev:server, dev:web, start scripts - dev runs both server and renderer in parallel via turbo - VITE_WS_URL injected so renderer connects to server in dev mode - turbo.json: Add WS env vars to globalEnv - vite.config.ts: Inject VITE_WS_URL at build time for WS server discovery - wsTransport.ts: Use VITE_WS_URL env var in dev, fall back to page URL in prod - Server dev script: Uses tsx for direct TypeScript execution with VITE_DEV_SERVER_URL Co-authored-by: Theo Browne <t3dotgg@users.noreply.github.com>
- Delete apps/desktop/ entirely (replaced by apps/server) - Remove IPC_CHANNELS constant from contracts (no longer needed) - Keep NativeApi interface (still the renderer's API contract) - Keep EDITORS constant (used by both server and renderer) - Update README.md with new architecture diagram and scripts - Update AGENTS.md with new package roles and architecture Co-authored-by: Theo Browne <t3dotgg@users.noreply.github.com>
- Tests welcome message on connect with cwd/projectName - Tests server.getConfig method - Tests providers.listSessions returns empty array - Tests error response for unknown methods - Tests graceful handling of invalid JSON - All 73 tests pass across 12 test files Co-authored-by: Theo Browne <t3dotgg@users.noreply.github.com>
- wsTransport.ts: Use addEventListener instead of on-function assignments - wsServer.ts: Replace filter(Boolean).pop() with direct array access - wsServer.test.ts: Use addEventListener for WebSocket events Co-authored-by: Theo Browne <t3dotgg@users.noreply.github.com>
|
Cursor Agent can help with this pull request. Just |
WalkthroughThe PR shifts the project from an Electron desktop app to a web-first architecture: removes the desktop app, adds a Node.js WebSocket server package, implements a renderer WebSocket transport and fallback NativeApi, updates contracts and build/config, and revises docs and CI accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser/Renderer
participant NativeApi as NativeApi (ws or preload)
participant WsTransport as WsTransport
participant WebSocket as WebSocket
participant Server as WsServer
Browser->>NativeApi: create/read API (readNativeApi)
alt browser fallback
NativeApi->>WsTransport: createWsNativeApi()
WsTransport->>WebSocket: new WebSocket(url)
WebSocket->>Server: open
Server->>WebSocket: push serverWelcome
WebSocket->>WsTransport: onmessage (push)
WsTransport->>Browser: onServerWelcome(payload)
end
Browser->>NativeApi: request('providers.startSession', params)
NativeApi->>WsTransport: request(...)
WsTransport->>WebSocket: send WsRequest (id)
WebSocket->>Server: receive request
Server->>Server: route -> ProviderManager
Server->>WebSocket: send WsResponse (id, result)
WebSocket->>WsTransport: onmessage (response)
WsTransport->>NativeApi: resolve request
NativeApi->>Browser: return result
Server->>WebSocket: push providers.event
WebSocket->>WsTransport: onmessage (push)
WsTransport->>Browser: dispatch event subscribers
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Comment |
Replace Electron desktop with a Node.js HTTP/WebSocket server and web UI for the Codex app, adding CLI
|
- wsNativeApi: Cache last welcome payload and replay to late subscribers. Fixes race where WS connect + welcome arrives before React effect fires. - App.tsx: Add bootstrappedRef to prevent duplicate project creation, skip bootstrap in Electron mode - wsTransport: Use standard import.meta.env for VITE_WS_URL access - CLAUDE.md: Fix header (was incorrectly '# AGENTS.md') Co-authored-by: Theo Browne <t3dotgg@users.noreply.github.com>
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| this.connect(); | ||
| } | ||
|
|
||
| async request<T = unknown>(method: string, params?: unknown): Promise<T> { |
There was a problem hiding this comment.
🟡 Medium
src/wsTransport.ts:36 window.location.port is empty on standard ports (80/443), producing an invalid URL like ws://example.com:. Consider conditionally appending the port only when present.
| async request<T = unknown>(method: string, params?: unknown): Promise<T> { | |
| : `ws://${window.location.hostname}${window.location.port ? `:${window.location.port}` : ''}`); |
🚀 Want me to fix this? Reply ex: "fix it for me".
| }); | ||
|
|
||
| // WebSocket server — upgrades from the HTTP server | ||
| const wss = new WebSocketServer({ server: httpServer }); |
There was a problem hiding this comment.
🟠 High
src/wsServer.ts:118 The WebSocketServer lacks origin validation, enabling Cross-Site WebSocket Hijacking (CSWSH). Any malicious website could connect to ws://localhost and invoke methods like shellOpenInEditor. Consider adding a verifyClient callback that checks the Origin header against an allowlist.
🚀 Want me to fix this? Reply ex: "fix it for me".
| } | ||
|
|
||
| const url = new URL(req.url ?? "/", `http://localhost:${port}`); | ||
| let filePath = path.join(staticDir, url.pathname); |
There was a problem hiding this comment.
🟢 Low
src/wsServer.ts:78 Consider decoding url.pathname with decodeURIComponent() before passing to path.join. Currently, percent-encoded characters like %20 are used literally, so files with spaces won't be found.
| let filePath = path.join(staticDir, url.pathname); | |
| let filePath = path.join(staticDir, decodeURIComponent(url.pathname)); |
🚀 Want me to fix this? Reply ex: "fix it for me".
| return new Promise((resolve, reject) => { | ||
| const server = net.createServer(); | ||
| server.listen(preferred, () => { | ||
| server.close(() => resolve(preferred)); |
There was a problem hiding this comment.
🟠 High
src/index.ts:18 If an external client connects to the temporary server, server.close() won't invoke its callback until that connection closes—causing startup to hang. Consider calling server.closeAllConnections() before server.close() to force-close any lingering sockets.
| server.close(() => resolve(preferred)); | |
| server.closeAllConnections(); | |
| server.close(() => resolve(preferred)); |
🚀 Want me to fix this? Reply ex: "fix it for me".
|
|
||
| // If not connected, wait for connection | ||
| const waitForOpen = () => { | ||
| const check = setInterval(() => { |
There was a problem hiding this comment.
🟡 Medium
src/wsTransport.ts:164 Offline send path: waitForOpen polls at 50ms and may send after a request times out. Suggest queuing messages and flushing on open; if polling remains, use a longer interval and check this.pending for the request before sending.
🚀 Want me to fix this? Reply ex: "fix it for me".
| } | ||
|
|
||
| const fileExt = path.extname(filePath); | ||
| const contentType = MIME_TYPES[fileExt] ?? "application/octet-stream"; |
There was a problem hiding this comment.
🟢 Low
src/wsServer.ts:103 Consider normalizing fileExt to lowercase before the MIME_TYPES lookup, since path.extname preserves case but the map uses lowercase keys.
🚀 Want me to fix this? Reply ex: "fix it for me".
Use 'tsx src/index.ts' instead of 'node --import tsx' for simpler dev execution Co-authored-by: Theo Browne <t3dotgg@users.noreply.github.com>
- Remove smoke-test script (was desktop-only) - Remove dist-electron from turbo outputs and .gitignore - Rename ELECTRON_RENDERER_PORT → PORT in vite.config.ts - Clean up turbo globalEnv (remove ELECTRON_RENDERER_PORT, WS_SERVER_PORT) - Update CI workflow to use bun 1.3.9 (matches package.json engines) Co-authored-by: Theo Browne <t3dotgg@users.noreply.github.com>
- Remove private:true so package can be published to npm - Add files field to limit published files to dist/ - Move @acme/contracts to devDependencies (bundled by tsup) - Add @acme/renderer devDependency for turbo build ordering - Add bundle-client.mjs: copies renderer build into dist/client/ - Update resolveStaticDir to check dist/client/ first (npx scenario), then fall back to monorepo layout (../../renderer/dist) - Full build pipeline: contracts → renderer → server + bundle client Co-authored-by: Theo Browne <t3dotgg@users.noreply.github.com>
| const DEFAULT_PORT = 3773; | ||
| const cwd = process.cwd(); | ||
|
|
||
| async function findAvailablePort(preferred: number): Promise<number> { |
There was a problem hiding this comment.
🟢 Low
src/index.ts:14 This has a TOCTOU race condition—the port may be claimed by another process between close() and createServer. Consider passing the already-bound server to createServer instead of just the port number, or document this as an acceptable risk given the narrow time window.
🚀 Want me to fix this? Reply ex: "fix it for me".
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
|
@greptileai review |
![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
🤖 Fix all issues with AI agents
In @.github/workflows/ci.yml:
- Line 20: Update the CI entry that sets "bun-version: 1.3.9" to ensure the test
matrix actually exercises the changed behaviors: add/modify a workflow step to
run "bun run test && bun run check" under bun-version: 1.3.9 and (optionally)
add a matrix axis or additional job that runs the same commands against the
previous stable version (e.g., 1.1.38) for comparison; explicitly ensure tests
cover code paths using fetch() and new WebSocket() (proxy behavior) and any HTTP
server request parsing/handlers (chunked encoding/request-smuggling scenarios)
so differences are detected before merging.
In `@AGENTS.md`:
- Line 1: The top-level header in the file does not match the filename:
AGENTS.md contains “# CLAUDE.md”; update the header to match the file's intent
or rename the file to CLAUDE.md. Specifically either change the markdown
top-line header from “# CLAUDE.md” to “# AGENTS” (or a more appropriate title)
inside AGENTS.md, or rename the file to CLAUDE.md so the filename and header are
consistent.
In `@apps/renderer/src/App.tsx`:
- Around line 31-92: The onServerWelcome callback registered inside
AutoProjectBootstrap captures stale closures of state.projects, state.threads
and state.activeThreadId; replace those captured accesses with refs that are
kept up-to-date (e.g., projectsRef, threadsRef, activeThreadIdRef) so the
callback reads current values when it runs, keep bootstrappedRef as-is, and
update the effect to reference and maintain these refs (synchronize them inside
a useEffect or on every dispatch) before using them in the onServerWelcome
handler to perform the find() checks and dispatch
ADD_PROJECT/ADD_THREAD/SET_ACTIVE_THREAD actions.
In `@apps/renderer/src/wsTransport.ts`:
- Around line 24-33: The constructor's default URL building can produce a
trailing colon and uses hardcoded "ws://" which breaks on HTTPS; change the
fallback for this.url (used in the constructor and before calling
this.connect()) to compute protocol = window.location.protocol === "https:" ?
"wss:" : "ws:" and use window.location.host (which already includes the port
when present) instead of window.location.hostname and window.location.port, so
the fallback becomes `${protocol}//${window.location.host}` when envUrl is not
provided.
In `@apps/server/src/wsServer.ts`:
- Around line 34-39: Update the ServerOptions interface to include an optional
host?: string with default '127.0.0.1', then use that host when starting the
server: locate the ServerOptions interface and add host, ensure any place that
reads options (e.g., the ws/http server bootstrap using options.port and
options.cwd) sets a default host = '127.0.0.1' during destructuring, and replace
plain httpServer.listen(port) calls with httpServer.listen(port, host) (also
update any other server.listen(...) invocations such as the provider/shell
endpoints) so the server binds to localhost by default while still allowing an
override.
- Line 15: The readyState check incorrectly uses the instance property
client.OPEN (undefined); change the import to bring in the WebSocket class
(default export) and use its static OPEN constant for comparisons. Update the
import to include WebSocket and WebSocketServer (so WebSocket.OPEN is available)
and replace any client.OPEN usage with WebSocket.OPEN in the readyState checks
(e.g., in the send-to-clients loop) so connected clients are detected correctly.
In `@README.md`:
- Around line 11-27: Add a language specifier to the fenced code block
containing the ASCII architecture diagram in README.md (e.g., use ```text or
```plaintext) so the markdown linter MD040 is satisfied; locate the fenced block
that starts with the ASCII box diagram (the Browser/Server/Codex diagram) and
replace the opening ``` with ```text (or ```plaintext) to mark it as plain text.
🧹 Nitpick comments (11)
apps/server/package.json (1)
17-17: Consider addingvitestto devDependencies.The
testscript usesvitest run, butvitestis not listed in this package's devDependencies. While it may resolve via workspace hoisting from the root, explicitly declaring it ensures reliable test execution and aligns with the practice of making package dependencies self-contained.📦 Proposed fix
"devDependencies": { "@acme/contracts": "workspace:*", "@acme/renderer": "workspace:*", "@types/node": "^22.10.2", "@types/ws": "^8.5.13", "tsup": "^8.3.5", "tsx": "^4.19.0", - "typescript": "^5.7.3" + "typescript": "^5.7.3", + "vitest": "^3.0.0" }package.json (1)
9-11: WebSocket URL is duplicated across scripts.
ws://localhost:3773appears in both thedevanddev:webscripts. If the default port changes, both locations need updating. Consider extracting this to a single source of truth or documenting the coupling.💡 Alternative approach using a shared variable
One option is to define the URL in a
.env.developmentfile or use a shell variable in a wrapper script. However, for simple cases, this inline approach is acceptable if the team is aware of the coupling.apps/server/src/wsServer.test.ts (1)
51-67:sendRequestcan hang indefinitely if the expected response never arrives.The
while (true)loop at Line 61 waits for a message with a matchingid, but if the server crashes, disconnects, or sends malformed responses, this will block forever and cause test timeouts to be uninformative.Consider adding a timeout or maximum iteration count:
🛡️ Proposed fix with timeout
async function sendRequest( ws: WebSocket, method: string, params?: unknown, ): Promise<WsResponse> { const id = crypto.randomUUID(); const message = JSON.stringify({ id, method, ...(params !== undefined ? { params } : {}) }); ws.send(message); + const deadline = Date.now() + 5000; // 5 second timeout // Wait for response with matching id while (true) { + if (Date.now() > deadline) { + throw new Error(`Timed out waiting for response to ${method}`); + } const parsed = (await waitForMessage(ws)) as Record<string, unknown>; if (parsed.id === id) { return parsed as WsResponse; } } }apps/renderer/src/wsNativeApi.ts (1)
10-12: Module-level state without cleanup mechanism.The singleton pattern uses module-level variables (
instance,welcomeListeners,lastWelcome) that persist for the lifetime of the page. While this is intentional for the singleton transport, there's nodisposeor reset mechanism. This could cause issues during hot module replacement (HMR) in development or if the app needs to reconnect with fresh state.Consider exposing a dispose function or ensuring HMR handles the module correctly:
💡 Optional: Add reset capability for HMR
// At module level, after instance declaration: if (import.meta.hot) { import.meta.hot.dispose(() => { instance?.transport.dispose(); instance = null; welcomeListeners.clear(); lastWelcome = null; }); }apps/server/src/index.ts (1)
14-35: Minor TOCTOU race condition in port detection.The
findAvailablePortfunction closes the test server before returning, creating a window where another process could claim the port. This is a known limitation of port probing and is generally acceptable for development tooling, but worth documenting.For a more robust approach in future iterations, consider passing the server instance directly to
createServeror usinghttpServer.listen(0)directly inwsServer.ts.packages/contracts/src/ws.ts (2)
5-26: Optional: align WS constants with camelCase naming.These exported constants are in SCREAMING_SNAKE_CASE, while repo conventions call for camelCase variable names. Consider renaming to
wsMethods/wsChannels(and updating imports) or documenting an exception for exported constants.Based on learnings: Applies to **/*.{ts,tsx} : Use camelCase for variable names.
30-58: Constrain method/channel values in schemas.
methodandchannelcurrently accept any non-empty string, so typos can pass validation. Consider derivingz.enumvalues fromWS_METHODS/WS_CHANNELSto keep the contract in sync.♻️ Proposed schema tightening
+const wsMethodSchema = z.enum(Object.values(WS_METHODS) as [string, ...string[]]); +const wsChannelSchema = z.enum(Object.values(WS_CHANNELS) as [string, ...string[]]); + export const wsRequestSchema = z.object({ id: z.string().min(1), - method: z.string().min(1), + method: wsMethodSchema, params: z.unknown().optional(), }); @@ export const wsPushSchema = z.object({ type: z.literal("push"), - channel: z.string().min(1), + channel: wsChannelSchema, data: z.unknown(), });apps/server/src/wsServer.ts (2)
1-1: Rename file to kebab-case (ws-server.ts).Repo conventions call for kebab-case filenames; consider renaming this file and updating imports accordingly.
Based on learnings: Applies to **/*.{ts,tsx} : Use kebab-case for file names.
19-32: Optional: camelCase the MIME types map.If you want to strictly follow the camelCase variable-name convention, consider renaming
MIME_TYPEStomimeTypesand updating its usage.♻️ Proposed rename
-const MIME_TYPES: Record<string, string> = { +const mimeTypes: Record<string, string> = { @@ - const contentType = MIME_TYPES[fileExt] ?? "application/octet-stream"; + const contentType = mimeTypes[fileExt] ?? "application/octet-stream";Based on learnings: Applies to **/*.{ts,tsx} : Use camelCase for variable names.
Also applies to: 102-104
apps/renderer/src/wsTransport.ts (2)
1-1: Rename file to kebab-case (ws-transport.ts).Repo conventions call for kebab-case filenames; consider renaming and updating imports.
Based on learnings: Applies to **/*.{ts,tsx} : Use kebab-case for file names.
11-12: Optional: camelCase timeout/backoff constants.If you want to follow the camelCase variable naming rule consistently, consider renaming
REQUEST_TIMEOUT_MSandRECONNECT_DELAYS_MS. The request/subscribe/dispose flow otherwise looks good.♻️ Proposed rename
-const REQUEST_TIMEOUT_MS = 60_000; -const RECONNECT_DELAYS_MS = [500, 1_000, 2_000, 4_000, 8_000]; +const requestTimeoutMs = 60_000; +const reconnectDelaysMs = [500, 1_000, 2_000, 4_000, 8_000]; @@ - }, REQUEST_TIMEOUT_MS); + }, requestTimeoutMs); @@ - setTimeout(() => clearInterval(check), REQUEST_TIMEOUT_MS); + setTimeout(() => clearInterval(check), requestTimeoutMs); @@ - const delay = - RECONNECT_DELAYS_MS[ - Math.min(this.reconnectAttempt, RECONNECT_DELAYS_MS.length - 1) - ] ?? RECONNECT_DELAYS_MS[0]!; + const delay = + reconnectDelaysMs[ + Math.min(this.reconnectAttempt, reconnectDelaysMs.length - 1) + ] ?? reconnectDelaysMs[0]!;Based on learnings: Applies to **/*.{ts,tsx} : Use camelCase for variable names.
Also applies to: 36-85, 156-187
| uses: oven-sh/setup-bun@v2 | ||
| with: | ||
| bun-version: 1.1.38 | ||
| bun-version: 1.3.9 |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Bun 1.3.9 release changelog breaking changes
💡 Result:
Bun v1.3.9 (published Feb 8, 2026) does not call out any explicit “breaking changes” section in the official changelog. The release is mostly new features, fixes, and perf work. [1]
Possible breaking / behavior-changing items to watch:
NO_PROXYis now respected even when you pass an explicitproxyoption tofetch()/new WebSocket()—requests that previously went through the proxy may now bypass it (or vice‑versa, depending on your env). [1]- Security fix: HTTP server chunked encoding parser issue that could lead to request smuggling (behavior may change for malformed/ambiguous requests). [1]
Not breaking, but notable changelog items:
bun run --parallel/--sequentialfor running multiple scripts (with workspace/filter integration). [1]bun:testmock()/spyOn()implementSymbol.dispose(works withusingto auto-restore). [1]--compilenow supports ESM bytecode (--bytecode --format=esm). [1]- Fix for SIGILL crashes on older ARMv8.0 aarch64 CPUs. [1]
Source
- [1] Bun v1.3.9 blog changelog (bun.com) (bun.com)
Citations:
🌐 Web query:
Bun version history 1.1.38 to 1.3.9 what changed
💡 Result:
Between Bun v1.1.38 (Nov 29, 2024) and Bun v1.3.9 (Feb 8, 2026), the highlights were:
- v1.1.38: bugfix release (VSCode terminal/debugger hang,
postgrescrash, TypeScript minification bug,fetch()rare crash,Bun.FileSystemRouter.reload()fix),reusePortforBun.listen/node:net, SQLite 3.45 → 3.47, plus other compatibility fixes. [1] - v1.2.3: major frontend tooling update—run
.htmldirectly as a dev server with hot reload/bundling; built-in routing forBun.serve()(routes);bun initReact template;bun install --analyze; plus Node.js compatibility and Bun.SQL improvements. [2] - v1.3.4: URLPattern API + fake timers in
bun:test(and other fixes). [3] - v1.3.5: Bun.Terminal (PTY) support via
Bun.spawn({ terminal: ... })for truly interactive subprocesses. [4] - v1.3.6: Bun.Archive (create/extract tar/tar.gz) + Bun.JSONC.parse() (JSON-with-comments) and related tooling updates. [5]
- v1.3.8: built-in
Bun.markdown(CommonMark-compliant parser) +bun build --metafile-mdfor Markdown bundle analysis output. [6] - v1.3.9:
bun run --parallel/--sequentialfor running multiple scripts;bun:testSymbol.disposesupport formock()/spyOn()(works withusing);NO_PROXYhonored even with explicit proxy options;--cpu-prof-interval; ESM bytecode support in--compile --bytecode; ARM crash fix on older ARMv8.0 CPUs; plus performance work (Markdown, AbortSignal) and a JavaScriptCore upgrade with multiple JIT/SIMD improvements. [7]
Sources: Bun blog release notes for v1.1.38, v1.2.3, v1.3.4, v1.3.5, v1.3.6, v1.3.8, v1.3.9. [1][2][3][4][5][6][7]
Confirm Bun 1.3.9 is tested against behavior changes before merging.
Bun 1.3.9 was published Feb 8, 2026 and is stable. However, the jump from 1.1.38 introduces two behavior-changing items to watch:
NO_PROXYis now respected even with an explicitproxyoption tofetch()/new WebSocket()—requests may route differently than before.- HTTP chunked encoding parser security fix for request smuggling—malformed/ambiguous requests will behave differently.
Ensure bun run test && bun run check exercise code paths affected by these changes.
🤖 Prompt for AI Agents
In @.github/workflows/ci.yml at line 20, Update the CI entry that sets
"bun-version: 1.3.9" to ensure the test matrix actually exercises the changed
behaviors: add/modify a workflow step to run "bun run test && bun run check"
under bun-version: 1.3.9 and (optionally) add a matrix axis or additional job
that runs the same commands against the previous stable version (e.g., 1.1.38)
for comparison; explicitly ensure tests cover code paths using fetch() and new
WebSocket() (proxy behavior) and any HTTP server request parsing/handlers
(chunked encoding/request-smuggling scenarios) so differences are detected
before merging.
| @@ -1,7 +1,7 @@ | |||
| # AGENTS.md | |||
| # CLAUDE.md | |||
There was a problem hiding this comment.
Filename and header mismatch.
The file is named AGENTS.md but the header reads # CLAUDE.md. This appears to be either an incomplete rename or a copy-paste issue. Consider renaming the file to CLAUDE.md or updating the header to match the current filename.
🤖 Prompt for AI Agents
In `@AGENTS.md` at line 1, The top-level header in the file does not match the
filename: AGENTS.md contains “# CLAUDE.md”; update the header to match the
file's intent or rename the file to CLAUDE.md. Specifically either change the
markdown top-line header from “# CLAUDE.md” to “# AGENTS” (or a more appropriate
title) inside AGENTS.md, or rename the file to CLAUDE.md so the filename and
header are consistent.
| function AutoProjectBootstrap() { | ||
| const { state, dispatch } = useStore(); | ||
| const bootstrappedRef = useRef(false); | ||
|
|
||
| useEffect(() => { | ||
| // Only relevant in browser mode — Electron doesn't send server welcome | ||
| if (isElectron) return; | ||
|
|
||
| return onServerWelcome((payload) => { | ||
| if (bootstrappedRef.current) return; | ||
|
|
||
| // Don't create duplicate projects for the same cwd | ||
| const existing = state.projects.find((p) => p.cwd === payload.cwd); | ||
| if (existing) { | ||
| bootstrappedRef.current = true; | ||
| // Ensure a thread is active | ||
| const existingThread = state.threads.find( | ||
| (t) => t.projectId === existing.id, | ||
| ); | ||
| if (existingThread && !state.activeThreadId) { | ||
| dispatch({ | ||
| type: "SET_ACTIVE_THREAD", | ||
| threadId: existingThread.id, | ||
| }); | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| bootstrappedRef.current = true; | ||
|
|
||
| // Create project + thread from server cwd | ||
| const projectId = crypto.randomUUID(); | ||
| dispatch({ | ||
| type: "ADD_PROJECT", | ||
| project: { | ||
| id: projectId, | ||
| name: payload.projectName, | ||
| cwd: payload.cwd, | ||
| model: DEFAULT_MODEL, | ||
| expanded: true, | ||
| }, | ||
| }); | ||
| dispatch({ | ||
| type: "ADD_THREAD", | ||
| thread: { | ||
| id: crypto.randomUUID(), | ||
| codexThreadId: null, | ||
| projectId, | ||
| title: "New thread", | ||
| model: DEFAULT_MODEL, | ||
| session: null, | ||
| messages: [], | ||
| events: [], | ||
| error: null, | ||
| createdAt: new Date().toISOString(), | ||
| }, | ||
| }); | ||
| }); | ||
| }, [state.projects, state.threads, state.activeThreadId, dispatch]); | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
Stale closure risk in onServerWelcome callback.
The useEffect captures state.projects, state.threads, and state.activeThreadId at registration time, but the onServerWelcome callback may fire asynchronously (or be replayed from cache) after the state has changed. This could cause the find() checks on Lines 43 and 47-49 to operate on stale data.
Consider using refs to access current state values, or restructure to avoid capturing state in the closure:
♻️ Suggested approach using refs
function AutoProjectBootstrap() {
const { state, dispatch } = useStore();
const bootstrappedRef = useRef(false);
+ const stateRef = useRef(state);
+ stateRef.current = state;
useEffect(() => {
// Only relevant in browser mode — Electron doesn't send server welcome
if (isElectron) return;
return onServerWelcome((payload) => {
if (bootstrappedRef.current) return;
+ const currentState = stateRef.current;
// Don't create duplicate projects for the same cwd
- const existing = state.projects.find((p) => p.cwd === payload.cwd);
+ const existing = currentState.projects.find((p) => p.cwd === payload.cwd);
if (existing) {
bootstrappedRef.current = true;
// Ensure a thread is active
- const existingThread = state.threads.find(
+ const existingThread = currentState.threads.find(
(t) => t.projectId === existing.id,
);
- if (existingThread && !state.activeThreadId) {
+ if (existingThread && !currentState.activeThreadId) {
dispatch({
type: "SET_ACTIVE_THREAD",
threadId: existingThread.id,
});
}
return;
}
// ... rest unchanged
});
- }, [state.projects, state.threads, state.activeThreadId, dispatch]);
+ }, [dispatch]);
return null;
}🤖 Prompt for AI Agents
In `@apps/renderer/src/App.tsx` around lines 31 - 92, The onServerWelcome callback
registered inside AutoProjectBootstrap captures stale closures of
state.projects, state.threads and state.activeThreadId; replace those captured
accesses with refs that are kept up-to-date (e.g., projectsRef, threadsRef,
activeThreadIdRef) so the callback reads current values when it runs, keep
bootstrappedRef as-is, and update the effect to reference and maintain these
refs (synchronize them inside a useEffect or on every dispatch) before using
them in the onServerWelcome handler to perform the find() checks and dispatch
ADD_PROJECT/ADD_THREAD/SET_ACTIVE_THREAD actions.
| constructor(url?: string) { | ||
| // In dev mode, VITE_WS_URL points to the server's WebSocket endpoint. | ||
| // In production, the page is served by the WS server on the same host:port. | ||
| const envUrl = import.meta.env.VITE_WS_URL as string | undefined; | ||
| this.url = | ||
| url ?? | ||
| (envUrl && envUrl.length > 0 | ||
| ? envUrl | ||
| : `ws://${window.location.hostname}:${window.location.port}`); | ||
| this.connect(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/renderer/src/wsTransport.ts | sed -n '20,40p'Repository: pingdotgg/codething-mvp
Length of output: 1034
🌐 Web query:
window.location.port empty default ports 80 443 HTTP HTTPS
💡 Result:
window.location.port is a string that contains the URL’s port only if it’s explicitly non-default. If the URL’s port is missing or is the default port for the scheme, browsers return the empty string "". [1][2]
http:/ws:default port → 80 →location.port === ""[1][3]https:/wss:default port → 443 →location.port === ""[1][3]
Examples:
http://example.com/→location.port === ""[2]http://example.com:80/→location.port === ""[2]http://example.com:8080/→location.port === "8080"[2]
If you need the “effective” port, you must infer it, e.g.:
const port = location.port || (location.protocol === "https:" ? "443" : "80");Sources: MDN Location.port, MDN URL.port, WHATWG URL Standard default-port table. [1][2][3]
🌐 Web query:
WebSocket wss protocol HTTPS ws protocol HTTP requirements
💡 Result:
-
ws://vswss://ws://is WebSocket over plain TCP (no TLS).wss://is WebSocket tunneled over TLS (encrypted), analogous to HTTPS. By default, WebSocket uses port 80 forwsand port 443 forwss. The client must complete a TLS handshake (and certificate verification) before the WebSocket handshake when usingwss. [1]
-
Relationship to HTTP/HTTPS (what’s actually required)
- WebSocket is its own protocol; the only HTTP part is the opening handshake, which is an HTTP/1.1 Upgrade request (typically
GET ... HTTP/1.1withUpgrade: websocket,Connection: Upgrade,Sec-WebSocket-Key,Sec-WebSocket-Version: 13, etc.). After the handshake succeeds (101 Switching Protocols), traffic is no longer HTTP. [1]
- WebSocket is its own protocol; the only HTTP part is the opening handshake, which is an HTTP/1.1 Upgrade request (typically
-
Browser security requirements (HTTPS page → WSS)
- If your web app is loaded over HTTPS, browsers generally block insecure subresource loads (mixed content). A
ws://connection from anhttps://page is treated as insecure (“mixed content”) and is typically blocked; usewss://instead. [2]
- If your web app is loaded over HTTPS, browsers generally block insecure subresource loads (mixed content). A
-
URL schemes you can pass in code
- The WHATWG WebSockets spec allows
ws,wss, and alsohttp/httpsin theWebSocket(url)constructor;httpmaps towsandhttpsmaps towssfor establishing the connection. [3]
- The WHATWG WebSockets spec allows
Practical checklist
- Serving site over HTTPS ⇒ expose WebSocket endpoint over
wss://with a valid TLS cert. [1][2] - Ensure your server/reverse proxy supports HTTP Upgrade (and forwards
Upgrade/Connectionheaders). [1]
Sources: RFC 6455 WebSocket Protocol [1], MDN Mixed Content [2], WHATWG WebSockets Standard [3].
Use wss/ws protocol detection and window.location.host for the default URL.
The current fallback produces an invalid URL on default ports—window.location.port returns an empty string for ports 80 and 443, resulting in ws://hostname: with a trailing colon. Additionally, hardcoded ws:// violates mixed-content policy on HTTPS pages and will be blocked by browsers; HTTPS-served pages must use wss://.
Suggested fix
- this.url =
- url ??
- (envUrl && envUrl.length > 0
- ? envUrl
- : `ws://${window.location.hostname}:${window.location.port}`);
+ const fallbackUrl = (() => {
+ const protocol = window.location.protocol === "https:" ? "wss" : "ws";
+ return `${protocol}://${window.location.host}`;
+ })();
+ this.url = url ?? (envUrl && envUrl.length > 0 ? envUrl : fallbackUrl);📝 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.
| constructor(url?: string) { | |
| // In dev mode, VITE_WS_URL points to the server's WebSocket endpoint. | |
| // In production, the page is served by the WS server on the same host:port. | |
| const envUrl = import.meta.env.VITE_WS_URL as string | undefined; | |
| this.url = | |
| url ?? | |
| (envUrl && envUrl.length > 0 | |
| ? envUrl | |
| : `ws://${window.location.hostname}:${window.location.port}`); | |
| this.connect(); | |
| constructor(url?: string) { | |
| // In dev mode, VITE_WS_URL points to the server's WebSocket endpoint. | |
| // In production, the page is served by the WS server on the same host:port. | |
| const envUrl = import.meta.env.VITE_WS_URL as string | undefined; | |
| const fallbackUrl = (() => { | |
| const protocol = window.location.protocol === "https:" ? "wss" : "ws"; | |
| return `${protocol}://${window.location.host}`; | |
| })(); | |
| this.url = url ?? (envUrl && envUrl.length > 0 ? envUrl : fallbackUrl); | |
| this.connect(); |
🤖 Prompt for AI Agents
In `@apps/renderer/src/wsTransport.ts` around lines 24 - 33, The constructor's
default URL building can produce a trailing colon and uses hardcoded "ws://"
which breaks on HTTPS; change the fallback for this.url (used in the constructor
and before calling this.connect()) to compute protocol =
window.location.protocol === "https:" ? "wss:" : "ws:" and use
window.location.host (which already includes the port when present) instead of
window.location.hostname and window.location.port, so the fallback becomes
`${protocol}//${window.location.host}` when envUrl is not provided.
| type WsResponse, | ||
| wsRequestSchema, | ||
| } from "@acme/contracts"; | ||
| import { WebSocketServer, type WebSocket } from "ws"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "wsServer.ts" -type f | head -20Repository: pingdotgg/codething-mvp
Length of output: 96
🏁 Script executed:
cat -n ./apps/server/src/wsServer.tsRepository: pingdotgg/codething-mvp
Length of output: 8834
🌐 Web query:
ws npm WebSocket OPEN constant static property
💡 Result:
In the ws npm package, the ready-state constants (including OPEN) are exposed as static properties on the WebSocket class, so you use them like WebSocket.OPEN (not ws.OPEN). The project’s README uses this pattern when checking client.readyState === WebSocket.OPEN. [1]
They match the standard WebSocket API constants: CONNECTING = 0, OPEN = 1, CLOSING = 2, CLOSED = 3. [2]
import WebSocket, { WebSocketServer } from "ws";
const wss = new WebSocketServer({ port: 8080 });
wss.on("connection", (socket) => {
if (socket.readyState === WebSocket.OPEN) {
socket.send("hi");
}
});
console.log(WebSocket.OPEN); // 1Sources: [1] [2]
Change import and use WebSocket.OPEN for readyState checks.
OPEN is a static constant on the WebSocket class, not an instance property. The current code at line 55 compares against client.OPEN (undefined), causing the condition to always fail and preventing provider pushes from being sent to connected clients.
Fix required
-import { WebSocketServer, type WebSocket } from "ws";
+import { WebSocketServer, WebSocket } from "ws";And at line 55:
- if (client.readyState === client.OPEN) {
+ if (client.readyState === WebSocket.OPEN) {📝 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.
| import { WebSocketServer, type WebSocket } from "ws"; | |
| import { WebSocketServer, WebSocket } from "ws"; |
🤖 Prompt for AI Agents
In `@apps/server/src/wsServer.ts` at line 15, The readyState check incorrectly
uses the instance property client.OPEN (undefined); change the import to bring
in the WebSocket class (default export) and use its static OPEN constant for
comparisons. Update the import to include WebSocket and WebSocketServer (so
WebSocket.OPEN is available) and replace any client.OPEN usage with
WebSocket.OPEN in the readyState checks (e.g., in the send-to-clients loop) so
connected clients are detected correctly.
| export interface ServerOptions { | ||
| port: number; | ||
| cwd: string; | ||
| staticDir?: string | undefined; | ||
| devUrl?: string | undefined; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "wsServer.ts" --path-separator /Repository: pingdotgg/codething-mvp
Length of output: 94
🏁 Script executed:
wc -l apps/server/src/wsServer.tsRepository: pingdotgg/codething-mvp
Length of output: 98
🏁 Script executed:
cat -n apps/server/src/wsServer.ts | head -100Repository: pingdotgg/codething-mvp
Length of output: 3627
🏁 Script executed:
cat -n apps/server/src/wsServer.ts | sed -n '100,150p'Repository: pingdotgg/codething-mvp
Length of output: 1738
🏁 Script executed:
cat -n apps/server/src/wsServer.ts | sed -n '220,251p'Repository: pingdotgg/codething-mvp
Length of output: 843
🏁 Script executed:
cat -n apps/server/src/wsServer.ts | sed -n '34,45p'Repository: pingdotgg/codething-mvp
Length of output: 479
🏁 Script executed:
# Let me check how httpServer.listen is being called currently and verify the Node.js behavior
cd apps/server && find . -name "*.ts" -o -name "*.js" | grep -E "(index|main|app)" | head -20Repository: pingdotgg/codething-mvp
Length of output: 81
🏁 Script executed:
cat -n apps/server/src/index.tsRepository: pingdotgg/codething-mvp
Length of output: 3936
Add a host option to bind the server to localhost by default, preventing unintended LAN exposure.
Currently, httpServer.listen(port) binds to all interfaces (0.0.0.0), exposing shell execution and provider control endpoints to the network. Add an optional host parameter defaulting to 127.0.0.1.
🔒 Suggested localhost binding
export interface ServerOptions {
port: number;
+ host?: string;
cwd: string;
staticDir?: string | undefined;
devUrl?: string | undefined;
}
@@
export function createServer(options: ServerOptions) {
- const { port, cwd, staticDir, devUrl } = options;
+ const { port, host = "127.0.0.1", cwd, staticDir, devUrl } = options;
@@
function start() {
return new Promise<void>((resolve) => {
- httpServer.listen(port, () => {
+ httpServer.listen(port, host, () => {
resolve();
});
});
}Also applies to lines 42 and 229–233.
📝 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.
| export interface ServerOptions { | |
| port: number; | |
| cwd: string; | |
| staticDir?: string | undefined; | |
| devUrl?: string | undefined; | |
| } | |
| export interface ServerOptions { | |
| port: number; | |
| host?: string; | |
| cwd: string; | |
| staticDir?: string | undefined; | |
| devUrl?: string | undefined; | |
| } |
🤖 Prompt for AI Agents
In `@apps/server/src/wsServer.ts` around lines 34 - 39, Update the ServerOptions
interface to include an optional host?: string with default '127.0.0.1', then
use that host when starting the server: locate the ServerOptions interface and
add host, ensure any place that reads options (e.g., the ws/http server
bootstrap using options.port and options.cwd) sets a default host = '127.0.0.1'
during destructuring, and replace plain httpServer.listen(port) calls with
httpServer.listen(port, host) (also update any other server.listen(...)
invocations such as the provider/shell endpoints) so the server binds to
localhost by default while still allowing an override.
| ``` | ||
| ┌─────────────────────────────────┐ | ||
| │ Browser (React + Vite) │ | ||
| │ Connected via WebSocket │ | ||
| └──────────┬──────────────────────┘ | ||
| │ ws://localhost:3773 | ||
| ┌──────────▼──────────────────────┐ | ||
| │ apps/server (Node.js) │ | ||
| │ WebSocket + HTTP static server │ | ||
| │ ProviderManager │ | ||
| │ CodexAppServerManager │ | ||
| └──────────┬──────────────────────┘ | ||
| │ JSON-RPC over stdio | ||
| ┌──────────▼──────────────────────┐ | ||
| │ codex app-server │ | ||
| └─────────────────────────────────┘ | ||
| ``` |
There was a problem hiding this comment.
Add language specifier to fenced code block.
The ASCII architecture diagram lacks a language specifier, which triggers a markdownlint warning (MD040). While text or plaintext would satisfy the linter, some renderers also support an empty language or none.
📝 Suggested fix
-```
+```text
┌─────────────────────────────────┐
│ Browser (React + Vite) │📝 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.
| ``` | |
| ┌─────────────────────────────────┐ | |
| │ Browser (React + Vite) │ | |
| │ Connected via WebSocket │ | |
| └──────────┬──────────────────────┘ | |
| │ ws://localhost:3773 | |
| ┌──────────▼──────────────────────┐ | |
| │ apps/server (Node.js) │ | |
| │ WebSocket + HTTP static server │ | |
| │ ProviderManager │ | |
| │ CodexAppServerManager │ | |
| └──────────┬──────────────────────┘ | |
| │ JSON-RPC over stdio | |
| ┌──────────▼──────────────────────┐ | |
| │ codex app-server │ | |
| └─────────────────────────────────┘ | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
[warning] 11-11: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In `@README.md` around lines 11 - 27, Add a language specifier to the fenced code
block containing the ASCII architecture diagram in README.md (e.g., use ```text
or ```plaintext) so the markdown linter MD040 is satisfied; locate the fenced
block that starts with the ASCII box diagram (the Browser/Server/Codex diagram)
and replace the opening ``` with ```text (or ```plaintext) to mark it as plain
text.
| function start() { | ||
| return new Promise<void>((resolve) => { | ||
| httpServer.listen(port, () => { | ||
| resolve(); | ||
| }); |
There was a problem hiding this comment.
🟠 High
src/wsServer.ts:257 Consider adding an error listener to httpServer before calling listen, rejecting the promise on failure (e.g., EADDRINUSE). Without it, binding errors crash the process instead of propagating through the returned Promise.
- function start() {
- return new Promise<void>((resolve) => {
- httpServer.listen(port, () => {
- resolve();
- });
+ function start() {
+ return new Promise<void>((resolve, reject) => {
+ httpServer.once("error", reject);
+ httpServer.listen(port, () => {
+ httpServer.off("error", reject);
+ resolve();
+ });🚀 Want me to fix this? Reply ex: "fix it for me".
🤖 Prompt for AI
In file apps/server/src/wsServer.ts around lines 257-261:
Consider adding an `error` listener to `httpServer` before calling `listen`, rejecting the promise on failure (e.g., `EADDRINUSE`). Without it, binding errors crash the process instead of propagating through the returned Promise.
| // Graceful shutdown | ||
| const shutdown = () => { | ||
| logger.info("shutting down"); | ||
| server.stop(); | ||
| process.exit(0); | ||
| }; |
There was a problem hiding this comment.
🟢 Low
src/index.ts:96 Consider awaiting server.stop() (or making it async) and adding a small delay before process.exit(0). The log file streams may not finish flushing before the process terminates, causing data loss.
- // Graceful shutdown
- const shutdown = () => {
- logger.info("shutting down");
- server.stop();
- process.exit(0);
- };
+ // Graceful shutdown
+ const shutdown = () => {
+ logger.info("shutting down");
+ server.stop();
+ // Allow streams to flush before exiting
+ setTimeout(() => process.exit(0), 100);
+ };🚀 Want me to fix this? Reply ex: "fix it for me".
🤖 Prompt for AI
In file apps/server/src/index.ts around lines 96-101:
Consider awaiting `server.stop()` (or making it async) and adding a small delay before `process.exit(0)`. The log file streams may not finish flushing before the process terminates, causing data loss.
3 participants
Overhaul architecture from Electron IPC to a Node.js WebSocket server and browser client for simpler packaging and launch.
This PR replaces the Electron-based architecture with a Node.js WebSocket server (
apps/server) that wraps the Codex app-server and serves the React web UI (apps/renderer). The browser client connects to this server via WebSocket, providing aNativeApiimplementation. This enables launching the application as a simplenpx t3command, which opens the web interface and automatically connects to the current working directory as a project.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores