New application architecture#9
Conversation
|
Cursor Agent can help with this pull request. Just |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
Replace Electron desktop app with
|
| if (parsed.data.type === "event") { | ||
| this.handleEvent(parsed.data); | ||
| } |
There was a problem hiding this comment.
🟢 Low
src/wsNativeApi.ts:175 If any listener in handleEvent throws, handleMessage rejects and becomes an unhandled promise rejection (since it's called with void). Consider wrapping the handleEvent call in a try-catch.
- if (parsed.data.type === "event") {
- this.handleEvent(parsed.data);
- }
+ if (parsed.data.type === "event") {
+ try {
+ this.handleEvent(parsed.data);
+ } catch {
+ // Prevent listener errors from causing unhandled rejections
+ }
+ }🚀 Want me to fix this? Reply ex: "fix it for me".
| function waitForProcessExit(processRef) { | ||
| return new Promise((resolve) => { | ||
| processRef.once("exit", (code) => resolve(code)); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🟢 Low
scripts/smoke-test.mjs:30 If the process exits before once('exit') is attached, the promise never resolves. Consider checking processRef.exitCode !== null first and resolving immediately if already exited.
| function waitForProcessExit(processRef) { | |
| return new Promise((resolve) => { | |
| processRef.once("exit", (code) => resolve(code)); | |
| }); | |
| } | |
| function waitForProcessExit(processRef) { | |
| return new Promise((resolve) => { | |
| if (processRef.exitCode !== null) { | |
| resolve(processRef.exitCode); | |
| return; | |
| } | |
| processRef.once("exit", (code) => resolve(code)); | |
| }); | |
| } |
🚀 Want me to fix this? Reply ex: "fix it for me".
| const api = readNativeApi(); | ||
| const { dispatch } = useStore(); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
🟡 Medium
src/App.tsx:32 BootstrapRouter only bootstraps once on mount. If the initial call fails and the runtime later becomes healthy, the app stays in an uninitialized state. Consider adding runtimeError to the effect dependencies and retrying bootstrap when the error clears, or consolidating retry logic with RuntimeHealthRouter.
🚀 Want me to fix this? Reply ex: "fix it for me".
| }; | ||
|
|
||
| void checkHealth(); | ||
| const interval = window.setInterval(() => { |
There was a problem hiding this comment.
🟡 Medium
src/App.tsx:89 If api.app.health() takes longer than 8 seconds, multiple calls can overlap and stale responses may overwrite newer state. Consider adding an AbortController or sequence ID to ignore results from previous ticks.
🚀 Want me to fix this? Reply ex: "fix it for me".
| } | ||
|
|
||
| private async request(method: string, params?: unknown) { | ||
| const socket = await this.connect(); |
There was a problem hiding this comment.
🟡 Medium
src/wsNativeApi.ts:76 The connect() call hangs indefinitely if the WebSocket stays in CONNECTING state (e.g., firewall silently dropping packets), since the timeout isn't set up until after connection succeeds. Consider wrapping connect() with its own timeout, or starting the request timeout before awaiting connection.
🚀 Want me to fix this? Reply ex: "fix it for me".
| }, | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🟡 Medium
src/wsNativeApi.ts:296 Consider using || instead of ?? here. An empty ?ws= query param returns "", which isn't nullish, so the fallback is skipped and new WebSocket("") throws.
| } | |
| return params.get("ws") || "ws://127.0.0.1:4317"; |
🚀 Want me to fix this? Reply ex: "fix it for me".
| return null; | ||
| } | ||
|
|
||
| private async handleMessage(raw: unknown) { |
There was a problem hiding this comment.
🟡 Medium
src/wsNativeApi.ts:170 Messages may process out of order because handleMessage is async but called without await. If a slow Blob.text() decode is followed by a fast string decode, the second message finishes first. Consider adding a message queue to preserve ordering, or document if out-of-order delivery is acceptable.
🚀 Want me to fix this? Reply ex: "fix it for me".
| session: bootstrap.session, | ||
| codexThreadId: bootstrap.session.threadId ?? entry.codexThreadId, | ||
| error: bootstrap.bootstrapError ?? entry.error, | ||
| } |
There was a problem hiding this comment.
🟡 BOOTSTRAP_FROM_SERVER preserves stale thread errors on successful re-bootstrap
When the runtime re-bootstraps successfully after a previous failure, stale error messages persist on existing threads instead of being cleared.
Root Cause
In the BOOTSTRAP_FROM_SERVER reducer case at apps/renderer/src/store.ts:377, the error field for an existing thread is set via:
error: bootstrap.bootstrapError ?? entry.error,When a bootstrap succeeds, bootstrap.bootstrapError is undefined (the field is optional in AppBootstrapResult at packages/contracts/src/ipc.ts:29). The nullish coalescing operator ?? then falls through to entry.error, preserving the old error.
Contrast this with the new thread path at apps/renderer/src/store.ts:353:
error: bootstrap.bootstrapError ?? null,Here, undefined ?? null correctly evaluates to null, clearing the error.
Impact: A user who encounters a bootstrap failure (e.g. Codex can't initialize) will continue to see the stale error banner on their thread even after the runtime reconnects successfully. The error is only cleared by creating a brand new thread. This is especially problematic because the RuntimeHealthRouter in App.tsx clears runtimeError on successful health checks, so the global "Offline" indicator disappears while the thread-level error persists — giving contradictory signals.
| } | |
| error: bootstrap.bootstrapError ?? null, |
Was this helpful? React with 👍 or 👎 to provide feedback.
| const exited = await Promise.race([ | ||
| waitForProcessExit(processRef).then(() => true), | ||
| new Promise((resolve) => { | ||
| setTimeout(() => resolve(false), timeoutMs); | ||
| }), | ||
| ]); |
There was a problem hiding this comment.
🟢 Low
scripts/smoke-test.mjs:41 The setTimeout at line 44 leaks if the process exits before the timeout fires. Consider clearing the timer when the race resolves.
- const exited = await Promise.race([
- waitForProcessExit(processRef).then(() => true),
- new Promise((resolve) => {
- setTimeout(() => resolve(false), timeoutMs);
- }),
- ]);
+ let timer;
+ const exited = await Promise.race([
+ waitForProcessExit(processRef).then(() => true),
+ new Promise((resolve) => {
+ timer = setTimeout(() => resolve(false), timeoutMs);
+ }),
+ ]);
+ clearTimeout(timer);🚀 Want me to fix this? Reply ex: "fix it for me".
🤖 Prompt for AI
In file apps/t3/scripts/smoke-test.mjs around lines 41-46:
The `setTimeout` at line 44 leaks if the process exits before the timeout fires. Consider clearing the timer when the race resolves.
| processRef.kill("SIGKILL"); | ||
| await waitForProcessExit(processRef); |
There was a problem hiding this comment.
🟢 Low
scripts/smoke-test.mjs:52 The SIGKILL fallback on line 53 awaits waitForProcessExit without a timeout, unlike the SIGTERM path. Consider adding a timeout here too to prevent indefinite hangs if the process fails to terminate.
- processRef.kill("SIGKILL");
- await waitForProcessExit(processRef);
+ processRef.kill("SIGKILL");
+ await Promise.race([
+ waitForProcessExit(processRef),
+ new Promise((resolve) => setTimeout(resolve, timeoutMs)),
+ ]);🚀 Want me to fix this? Reply ex: "fix it for me".
🤖 Prompt for AI
In file apps/t3/scripts/smoke-test.mjs around lines 52-53:
The `SIGKILL` fallback on line 53 awaits `waitForProcessExit` without a timeout, unlike the `SIGTERM` path. Consider adding a timeout here too to prevent indefinite hangs if the process fails to terminate.
Co-authored-by: Theo Browne <t3dotgg@users.noreply.github.com>
Co-authored-by: Theo Browne <t3dotgg@users.noreply.github.com>
Co-authored-by: Theo Browne <t3dotgg@users.noreply.github.com>
Co-authored-by: Theo Browne <t3dotgg@users.noreply.github.com>
Co-authored-by: Theo Browne <t3dotgg@users.noreply.github.com>
Co-authored-by: Theo Browne <t3dotgg@users.noreply.github.com>
Co-authored-by: Theo Browne <t3dotgg@users.noreply.github.com>
Co-authored-by: Theo Browne <t3dotgg@users.noreply.github.com>
Co-authored-by: Theo Browne <t3dotgg@users.noreply.github.com>
Co-authored-by: Theo Browne <t3dotgg@users.noreply.github.com>
Co-authored-by: Theo Browne <t3dotgg@users.noreply.github.com>
Co-authored-by: Theo Browne <t3dotgg@users.noreply.github.com>
Co-authored-by: Theo Browne <t3dotgg@users.noreply.github.com>
Co-authored-by: Theo Browne <t3dotgg@users.noreply.github.com>
Co-authored-by: Theo Browne <t3dotgg@users.noreply.github.com>
Co-authored-by: Theo Browne <t3dotgg@users.noreply.github.com>
Co-authored-by: Theo Browne <t3dotgg@users.noreply.github.com>
| const fakeCodex = createFakeCodexAppServerBinary(); | ||
| const distCli = path.join(appRoot, "dist", "cli.js"); | ||
| if (!fs.existsSync(distCli)) { | ||
| throw new Error("Missing dist/cli.js. Run `bun run --cwd apps/t3 build` first."); | ||
| } |
There was a problem hiding this comment.
🟢 Low
scripts/smoke-test.mjs:573 If dist/cli.js is missing, the temporary directory created by createFakeCodexAppServerBinary() leaks because the throw happens before the try block. Consider moving the existence check before creating the fake binary, or wrapping the check inside the try.
- const fakeCodex = createFakeCodexAppServerBinary();
- const distCli = path.join(appRoot, "dist", "cli.js");
- if (!fs.existsSync(distCli)) {
- throw new Error("Missing dist/cli.js. Run `bun run --cwd apps/t3 build` first.");
- }
+ const distCli = path.join(appRoot, "dist", "cli.js");
+ if (!fs.existsSync(distCli)) {
+ throw new Error("Missing dist/cli.js. Run `bun run --cwd apps/t3 build` first.");
+ }
+ const fakeCodex = createFakeCodexAppServerBinary();🚀 Want me to fix this? Reply ex: "fix it for me".
🤖 Prompt for AI
In file apps/t3/scripts/smoke-test.mjs around lines 573-577:
If `dist/cli.js` is missing, the temporary directory created by `createFakeCodexAppServerBinary()` leaks because the `throw` happens before the `try` block. Consider moving the existence check before creating the fake binary, or wrapping the check inside the `try`.
Co-authored-by: Theo Browne <t3dotgg@users.noreply.github.com>
Co-authored-by: Theo Browne <t3dotgg@users.noreply.github.com>
Co-authored-by: Theo Browne <t3dotgg@users.noreply.github.com>
Co-authored-by: Theo Browne <t3dotgg@users.noreply.github.com>
Co-authored-by: Theo Browne <t3dotgg@users.noreply.github.com>
Co-authored-by: Theo Browne <t3dotgg@users.noreply.github.com>
Co-authored-by: Theo Browne <t3dotgg@users.noreply.github.com>
…eally completes A backgrounded/async `Agent` (Task) subagent returns its tool_result immediately -- an "Async agent launched successfully. agentId: <id>" receipt, not the subagent's completion. t3code maps that ack to tool.completed on the Agent tool_use, which is also the root of the subagent tree; the bug pingdotgg#9 terminal-latch then pinned the ref "finished" the instant the agent was dispatched, while the real work ran on -- and was correctly tracked by -- the native task.* stream (which is why the parent Background pill stayed correct). Fix, confined to ProjectionSnapshotQuery.getSubagentTree (parent timeline untouched): detect the launch-ack collab row, extract the embedded agentId (== taskId), and DON'T latch it as terminal. Resolve a backgrounded ref's status from the task.* stream instead -- inProgress until the real task.completed, then completed/failed. Synchronous subagents keep the exact bug pingdotgg#9 latch behavior. The task.completed signal is a persisted activity row, so the tree ref resolves correctly even across a daemon restart (no ledger-style restart staleness). TDD: 3 new getSubagentTree tests (still-running -> inProgress, task.completed -> completed, failed -> failed); bug pingdotgg#9 regression preserved; 262 related tests green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
One codex app-server process multiplexes many app threads (the opener plus every native subagent child), but the protocol logger froze the opener's threadId at open time, so all traffic landed in the opener's log file — and rotation of that busy file destroyed other threads' native ground truth (audit plan #9, threads c878541b/de5f191a/68f7595b/ af66fc2c had no log file at all). The logger now resolves the target per frame: it extracts the native thread id from each frame and looks it up in a per-session route map seeded when a root turn or subagent thread registers, falling back to the opener for unrouted frames. App-verified with a real codex subagent spawn: the two subagent native threads each got their own dedicated log file containing only their own traffic (94 and 54 frames), where previously everything multiplexed into the parent's file. Unit tests cover native-thread-id extraction across decoded/raw frame shapes and the two-thread routing decision. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Replaces the Electron app with a Node.js CLI (
t3) that launches a WebSocket server and opens a browser-based UI to simplify deployment and improve accessibility.This PR implements the requested architectural overhaul, migrating from an Electron-based application with IPC to a Node.js runtime (
apps/t3) that hosts a WebSocket API and serves a browser-based frontend (apps/renderer). Theapps/desktoppackage has been refactored intoapps/runtimeto contain the core provider and process management logic, now consumed by the new Node.js server. Thenpx t3command now builds the frontend, starts the local server, automatically opens the web interface, and initializes the current working directory as a project with a new session. Electron and its related dependencies have been entirely removed.