port desktop app to Effect#2546
Open
juliusmarminge wants to merge 33 commits intomainfrom
Open
Conversation
- Replace promise-based readiness polling with Effect layers and services - Update desktop startup flow to use URL-backed readiness state - Add Effect-based tests for HTTP and startup readiness
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
size:L
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Timeout silently succeeds instead of failing with error
- Added Option.match after Effect.timeoutOption to convert Option.None (timeout) into a BackendTimeoutError failure instead of silently succeeding, matching the established pattern used in all other timeoutOption call sites in the codebase.
Or push these changes by commenting:
@cursor push 27f2b65c9b
Preview (27f2b65c9b)
diff --git a/apps/desktop/src/backendReadiness.ts b/apps/desktop/src/backendReadiness.ts
--- a/apps/desktop/src/backendReadiness.ts
+++ b/apps/desktop/src/backendReadiness.ts
@@ -3,6 +3,7 @@
import * as Data from "effect/Data";
import * as Duration from "effect/Duration";
import * as Effect from "effect/Effect";
+import * as Option from "effect/Option";
import * as Predicate from "effect/Predicate";
import * as Schedule from "effect/Schedule";
import { HttpClient } from "effect/unstable/http";
@@ -60,9 +61,13 @@
yield* client.get(requestUrl).pipe(
Effect.asVoid,
Effect.timeoutOption(timeout),
+ Effect.flatMap(
+ Option.match({
+ onNone: () => Effect.fail(new BackendTimeoutError({ url: baseUrl })),
+ onSome: () => Effect.void,
+ }),
+ ),
Effect.catchTags({
- TimeoutError: () => Effect.fail(new BackendTimeoutError({ url: baseUrl })),
- // Maybe map this to different error kind?
HttpClientError: () => Effect.fail(new BackendTimeoutError({ url: baseUrl })),
}),
);You can send follow-ups to the cloud agent here.
Contributor
ApprovabilityVerdict: Needs human review 1 blocking correctness issue found. Diff is too large for automated approval analysis. A human reviewer should evaluate this PR. You can customize Macroscope's approvability policy. Learn more. |
Member
Author
Effect.timeoutOption returns Option.None on timeout (on the success channel), not a TimeoutError on the error channel. The previous code discarded the Option result, so a timeout was treated as success. Now we explicitly match the Option: None maps to BackendTimeoutError, Some maps to Effect.void. The now-unreachable TimeoutError catchTag is removed. Applied via @cursor push command
macroscopeapp
Bot
dismissed
their stale review
Dismissing prior approval to re-evaluate d8f0d72
- Spawn the backend through a shared Effect runner with fd3 bootstrap - Replace ad hoc readiness/listening plumbing with HTTP readiness hooks - Add contract support for desktop bootstrap payloads
macroscopeapp
Bot
dismissed
their stale review
Dismissing prior approval to re-evaluate b5c8ea4
github-actions
Bot
added
size:XL
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Port fields lose range validation in bootstrap schema
- Replaced Schema.Number with a shared PortSchema (Schema.Int with isBetween 1-65535) for both port and tailscaleServePort fields in DesktopBackendBootstrap, restoring the validation that was present in the old BootstrapEnvelopeSchema.
- ✅ Fixed: Readiness timeout fires but no window ever created
- Added fallback window creation in the onReadinessFailure handler so users get a window (showing reconnection state) instead of being stuck with no UI when the readiness probe times out.
Or push these changes by commenting:
@cursor push 1291f41e07
Preview (1291f41e07)
diff --git a/apps/desktop/src/main.ts b/apps/desktop/src/main.ts
--- a/apps/desktop/src/main.ts
+++ b/apps/desktop/src/main.ts
@@ -1496,6 +1496,12 @@
`bootstrap backend readiness warning message=${formatErrorMessage(error)}`,
);
console.warn("[desktop] backend readiness check failed during bootstrap", error);
+
+ if (isDevelopment) return;
+ const existingWindow = mainWindow ?? BrowserWindow.getAllWindows()[0] ?? null;
+ if (existingWindow !== null) return;
+ mainWindow = createWindow();
+ writeDesktopLogHeader("bootstrap main window created (readiness timeout fallback)");
}),
onOutput: (streamName, chunk) =>
Effect.sync(() => {
diff --git a/apps/server/src/cli.ts b/apps/server/src/cli.ts
--- a/apps/server/src/cli.ts
+++ b/apps/server/src/cli.ts
@@ -5,6 +5,7 @@
CommandId,
DesktopBackendBootstrap,
OrchestrationReadModel,
+ PortSchema,
ProjectId,
type ClientOrchestrationCommand,
} from "@t3tools/contracts";
@@ -67,8 +68,6 @@
import { WorkspacePaths } from "./workspace/Services/WorkspacePaths.ts";
import { WorkspacePathsLive } from "./workspace/Layers/WorkspacePaths.ts";
-const PortSchema = Schema.Int.check(Schema.isBetween({ minimum: 1, maximum: 65535 }));
-
const modeFlag = Flag.choice("mode", RuntimeMode.literals).pipe(
Flag.withDescription("Runtime mode. `desktop` keeps loopback defaults unless overridden."),
Flag.optional,
diff --git a/packages/contracts/src/baseSchemas.ts b/packages/contracts/src/baseSchemas.ts
--- a/packages/contracts/src/baseSchemas.ts
+++ b/packages/contracts/src/baseSchemas.ts
@@ -5,6 +5,7 @@
export const NonNegativeInt = Schema.Int.check(Schema.isGreaterThanOrEqualTo(0));
export const PositiveInt = Schema.Int.check(Schema.isGreaterThanOrEqualTo(1));
+export const PortSchema = Schema.Int.check(Schema.isBetween({ minimum: 1, maximum: 65535 }));
export const IsoDateTime = Schema.String;
export type IsoDateTime = typeof IsoDateTime.Type;
diff --git a/packages/contracts/src/desktopBootstrap.ts b/packages/contracts/src/desktopBootstrap.ts
--- a/packages/contracts/src/desktopBootstrap.ts
+++ b/packages/contracts/src/desktopBootstrap.ts
@@ -1,14 +1,16 @@
import { Schema } from "effect";
+import { PortSchema } from "./baseSchemas.ts";
+
export const DesktopBackendBootstrap = Schema.Struct({
mode: Schema.Literal("desktop"),
noBrowser: Schema.Boolean,
- port: Schema.Number,
+ port: PortSchema,
t3Home: Schema.String,
host: Schema.String,
desktopBootstrapToken: Schema.String,
tailscaleServeEnabled: Schema.Boolean,
- tailscaleServePort: Schema.Number,
+ tailscaleServePort: PortSchema,
otlpTracesUrl: Schema.optional(Schema.String),
otlpMetricsUrl: Schema.optional(Schema.String),
});You can send follow-ups to the cloud agent here.
- Convert backend port and persistence flows to Effect - Add desktop environment, logging, shutdown, and backend manager modules - Expand tests around networking and saved state handling
github-actions
Bot
added
size:XXL
Contributor
|
Bugbot Autofix prepared a fix for the issue found in the latest run.
Or push these changes by commenting: Preview (fbac652b11)diff --git a/apps/desktop/src/desktopBackendManager.ts b/apps/desktop/src/desktopBackendManager.ts
--- a/apps/desktop/src/desktopBackendManager.ts
+++ b/apps/desktop/src/desktopBackendManager.ts
@@ -232,11 +232,11 @@
.exists(config.entryPath)
.pipe(Effect.orElseSucceed(() => false));
+ yield* cancelRestart;
yield* Ref.update(state, (latest) => ({
...latest,
desiredRunning: true,
ready: false,
- restartFiber: Option.none(),
}));
if (!entryExists) {You can send follow-ups to the cloud agent here. |
juliusmarminge
changed the title
Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: codex <codex@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Restart backoff defeated by premature attempt counter reset
- Moved the
restartAttempt: 0reset fromonStartedtoonReadyso the backoff counter only resets after the backend process proves healthy via the HTTP readiness check.
- Moved the
Or push these changes by commenting:
@cursor push e48c9bf69f
Preview (e48c9bf69f)
diff --git a/apps/desktop/src/backend/DesktopBackendManager.ts b/apps/desktop/src/backend/DesktopBackendManager.ts
--- a/apps/desktop/src/backend/DesktopBackendManager.ts
+++ b/apps/desktop/src/backend/DesktopBackendManager.ts
@@ -422,16 +422,13 @@
...run,
pid: Option.some(pid),
}));
- yield* Ref.update(state, (latest) => ({
- ...latest,
- restartAttempt: 0,
- }));
yield* events.onStarted({ pid, config });
}),
onReady: () =>
Effect.gen(function* () {
yield* Ref.update(state, (latest) => ({
...latest,
+ restartAttempt: 0,
ready: Option.match(latest.active, {
onNone: () => latest.ready,
onSome: (run) => (run.id === runId ? true : latest.ready),You can send follow-ups to the cloud agent here.
- Replace ad hoc failures with typed desktop errors - Switch desktop tests and runtime layers to NodeServices - Tighten Effect typings across desktop, server, and shared code
- Switch schemas and helpers to `effect/Schema`, `Effect`, and related subpaths - Update contract tests to match the new import style
- Switch Effect usage to submodule imports for language-service compliance - Update package tsconfigs and narrow generated-script inclusion - Adjust related tests and helpers for the new import style
- Capture the current effect context before registering the window-close handler - Run pending prompt cleanup through the captured fork to preserve context
- Promote anyUnknownInErrorContext to error across packages - Include scripts and test files in effect package typechecks
- Derive desktop home from the injected environment instead of config fallbacks - Move backend lifecycle hooks into shared app services and simplify tests - Consolidate desktop runtime ignore rules at the repo root
- Return null when secret decryption throws - Call Electron loadURL/devtools directly instead of wrapping sync effects
- Remove the contract import - Use the literal desktop SSH cancel result channel
- Defer Electron menu popup effects until execution time - Avoid persisting unchanged update channels - Share port validation in contracts and bootstrap schemas - Tighten backend restart state and add coverage
Comment on lines
+140
to
+144
| popupTemplate: (input) => | ||
| Effect.sync(() => { | ||
| if (input.template.length === 0) { | ||
| return Effect.void; | ||
| } |
Contributor
There was a problem hiding this comment.
🟠 High electron/ElectronMenu.ts:140
In popupTemplate, when input.template.length === 0, the code returns Effect.void from inside Effect.sync. Effect.sync expects its callback to return a plain value that becomes the effect's success value; returning Effect.void makes that object itself the success value rather than executing it. Consider using return (or restructuring with Effect.when or Effect.flatMap) to properly handle the empty case.
- if (input.template.length === 0) {
- return Effect.void;
- }
+ if (input.template.length === 0) {
+ return;
+ }🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/desktop/src/electron/ElectronMenu.ts around lines 140-144:
In `popupTemplate`, when `input.template.length === 0`, the code returns `Effect.void` from inside `Effect.sync`. `Effect.sync` expects its callback to return a plain value that becomes the effect's success value; returning `Effect.void` makes that object itself the success value rather than executing it. Consider using `return` (or restructuring with `Effect.when` or `Effect.flatMap`) to properly handle the empty case.
Evidence trail:
apps/desktop/src/electron/ElectronMenu.ts lines 140-146 (REVIEWED_COMMIT): `Effect.sync(() => { if (input.template.length === 0) { return Effect.void; } ... })` - Effect.void returned as a plain value inside Effect.sync callback. Line 32: return type is `Effect.Effect<void>`. Effect.sync takes `() => A` and returns `Effect<A>`, so returning `Effect.void` yields `Effect<Effect<void>>` conceptually.
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Returning Effect value from sync thunk instead of void
- Replaced
return Effect.voidwith a plainreturninside the Effect.sync thunk so the early exit path correctly returns undefined rather than an unexecuted Effect object.
- Replaced
Or push these changes by commenting:
@cursor push 16417b5abc
Preview (16417b5abc)
diff --git a/apps/desktop/src/electron/ElectronMenu.ts b/apps/desktop/src/electron/ElectronMenu.ts
--- a/apps/desktop/src/electron/ElectronMenu.ts
+++ b/apps/desktop/src/electron/ElectronMenu.ts
@@ -140,7 +140,7 @@
popupTemplate: (input) =>
Effect.sync(() => {
if (input.template.length === 0) {
- return Effect.void;
+ return;
}
Electron.Menu.buildFromTemplate([...input.template]).popup({ window: input.window });
}),You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit f57d34a. Configure here.
| return Effect.void; | ||
| } | ||
| Electron.Menu.buildFromTemplate([...input.template]).popup({ window: input.window }); | ||
| }), |
Contributor
There was a problem hiding this comment.
Returning Effect value from sync thunk instead of void
Low Severity
The popupTemplate implementation uses return Effect.void inside Effect.sync(), which returns an unexecuted Effect object as the success value of the sync thunk rather than performing a no-op. The developer likely intended a plain return (or return undefined) to exit early when the template is empty. While the behavior is correct (nothing happens for empty templates), this pattern is misleading — it looks like it's trying to "run" Effect.void but actually just produces a dead value that's discarded by the caller.
Reviewed by Cursor Bugbot for commit f57d34a. Configure here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
URLhandling for backend endpoints and the new readiness error type.Testing
bun fmtbun lintbun typecheckbun run testforapps/desktop/src/backendReadiness.test.tsandapps/desktop/src/backendStartupReadiness.test.tsNote
High Risk
Large refactor of the Electron desktop main process and backend startup/lifecycle into new Effect services; failures here can prevent the app from booting, spawning the backend, or shutting down cleanly.
Overview
Ports the desktop app’s Electron main process to an Effect-based layered architecture, replacing ad-hoc startup code with
DesktopApp.programplus composed runtime layers for environment, lifecycle, windowing, IPC, SSH, updates, and logging.Reworks backend startup into an Effect-managed
DesktopBackendManager/DesktopBackendConfigurationflow: selects/validates backend port, spawns the server with a schema-encoded bootstrap payload over an extra FD, polls HTTP readiness with retries/timeouts, and adds restart-on-crash plus structured session/output logging.Introduces Effect wrappers for key Electron APIs (
ElectronApp,ElectronDialog,ElectronMenu,ElectronProtocol,ElectronShell,ElectronTheme,ElectronUpdater,ElectronSafeStorage) and adds broad@effect/vitesttest coverage for identity/environment resolution, protocol/menu/dialog behavior, updater error typing, and backend spawn/readiness/restart scenarios. Also consolidates Electron build artefact ignores into the repo root.gitignoreand updates desktop devDependencies accordingly.Reviewed by Cursor Bugbot for commit f57d34a. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Port the Electron desktop app main process to an Effect-based layered architecture
apps/desktop/src/main.tswith Effect service layers composed and run viaNodeRuntime.runMain, covering window management, IPC, auto-updater, SSH, backend process lifecycle, server exposure, and app identity.Context.Servicetypes acrossapps/desktop/src/:ElectronApp,ElectronWindow,ElectronDialog,ElectronMenu,ElectronTheme,ElectronUpdater,ElectronShell,ElectronSafeStorage,ElectronProtocol,DesktopBackendManager,DesktopBackendConfiguration,DesktopSettingsState,DesktopServerExposure,DesktopSshEnvironment,DesktopSshPasswordPrompts,DesktopUpdates,DesktopWindow,DesktopAppIdentity,DesktopLifecycle,DesktopRun,DesktopState, andDesktopIpc.DesktopBackendBootstrapschema topackages/contracts/src/desktopBootstrap.tsand a comprehensive set of IPC contract schemas inpackages/contracts/src/ipc.tsfor runtime encoding/decoding.apps/desktop/src/ipc/channels.ts; the preload bridge now sends object-shaped payloads for several channels that previously used positional arguments.packages/contracts,packages/shared,packages/ssh,packages/tailscale, andpackages/effect-acpimports from barrel-style to namespace-style to comply with@effect/language-servicerules, and upgradesanyUnknownInErrorContextdiagnostics to errors across multipletsconfig.jsonfiles.ENSURE_SSH_ENVIRONMENT,SET_SAVED_ENVIRONMENT_SECRET,FETCH_SSH_ENVIRONMENT_DESCRIPTOR, andshowContextMenuis a breaking change for any renderer code not updated in this PR.Macroscope summarized f57d34a.