feat(desktop): land Electron desktop app (consolidated stack)#397
Conversation
Greptile SummaryThis PR lands the consolidated Electron desktop stack for Kanban: the runtime orchestrator ( Confidence Score: 5/5Safe to merge; the single finding is a hardening suggestion, not a current defect. All findings are P2. The missing fetch timeout in relayOAuthCallback is a best-practice gap rather than a present defect — the relay targets localhost and the runtime is only called when getUrl() is non-null. No P0/P1 issues identified. packages/desktop/src/oauth-relay.ts — minor fetch-timeout hardening suggested.
|
| Filename | Overview |
|---|---|
| packages/desktop/src/main.ts | Main Electron entry: wires orchestrator, window factory, protocol handler, IPC, and lifecycle hooks. The before-quit orphan-window gap (isOwned() false during spawn) is a pre-existing open thread; rest of the logic looks correct with well-guarded async paths. |
| packages/desktop/src/runtime-orchestrator.ts | Core orchestrator: all previously flagged races (terminated guard, dispose null-guard, orphan-cleanup capture, restart stale URL, startRecoveryProbe ordering) appear addressed. Generation tokens for in-flight probes are a clean approach. |
| packages/desktop/src/oauth-relay.ts | OAuth relay: retry loop is clean, but missing AbortController/timeout on fetch unlike the checkHealth pattern; could stall unbounded on each attempt if the runtime is slow. |
| packages/desktop/src/protocol-handler.ts | Protocol parsing correctly handles non-special kanban:// scheme by rejoining hostname+pathname; registration guard prevents double-registration. |
| packages/desktop/scripts/notarize.cjs | afterSign hook: correctly checks electronPlatformName (darwin vs mac), validates required env vars before importing @electron/notarize; exports helpers for unit testing. |
| packages/desktop/electron-builder.yml | Packaging config: asarUnpack covers cli/** and node-pty for real-path requirements; extraResources filter explicitly excludes kanban-dev shims; DMG artifact naming is correct. |
Sequence Diagram
sequenceDiagram
participant App as main.ts
participant Orch as RuntimeOrchestrator
participant Child as RuntimeChildManager
participant Runtime as Kanban Runtime (CLI)
participant Browser as BrowserWindow
App->>Orch: connect()
Orch->>Runtime: checkHealth(origin)
alt Runtime already running
Runtime-->>Orch: 200 OK
Orch->>Orch: setUrl(origin, owns=false)
Orch->>Orch: startAttachedProbe()
else No runtime found
Runtime-->>Orch: error/timeout
Orch->>Child: createManager() + start()
Child->>Runtime: spawn CLI shim
Runtime-->>Child: ready URL
Child-->>Orch: url
Orch->>Orch: setUrl(url, owns=true)
end
Orch-->>App: emit url-changed(url)
App->>Browser: loadUrlInAllWindows(url)
note over Orch,Runtime: Crash / detach path
Runtime--xChild: process exit
Child-->>Orch: emit crashed
Orch->>Orch: handleCrash() → startRecoveryProbe()
Orch->>Orch: setUrl(null)
Orch-->>App: emit crashed
App->>Browser: showDisconnectedScreen()
note over App,Orch: Quit path
App->>Orch: shutdown() [if owned]
Orch->>Orch: terminated=true, drain promises
Orch->>Child: manager.shutdown()
Child->>Runtime: SIGTERM → SIGKILL
Orch->>App: returns
App->>App: app.quit()
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
packages/desktop/src/oauth-relay.ts:19-21
**No fetch timeout in `relayOAuthCallback`**
Unlike `checkHealth` — which wraps its `fetch` with an `AbortController` — `relayOAuthCallback` issues bare `fetch` calls with no timeout. With the default `retries = 2`, three consecutive requests can each hang indefinitely before the 1-second inter-attempt delay even begins, stalling the OAuth flow silently for an unbounded duration if the runtime is momentarily unresponsive.
Suggested pattern matching `checkHealth`:
```typescript
const controller = new AbortController();
const timer = setTimeout(() => controller.abort(), 5_000);
try {
const response = await deps.fetch(relayUrl, { signal: controller.signal });
if (response.ok) return;
lastFailure = `HTTP ${response.status}`;
} catch (err) {
lastFailure = err instanceof Error ? err.message : String(err);
} finally {
clearTimeout(timer);
}
```
Reviews (18): Last reviewed commit: "fix(desktop): address remaining review c..." | Re-trigger Greptile
d2d70ed to
05794f5
Compare
15261ba to
65b59bd
Compare
ffc697e to
3a2e830
Compare
56cb70a to
874a24a
Compare
8de6991 to
f6cca91
Compare
- Extract `shutdownTimeoutMs` into a `DEFAULT_CHILD_SHUTDOWN_TIMEOUT_MS` constant next to the other orchestrator timing defaults so all knobs live together and the magic literal `5_000` in `createManager()` is no longer a scanning hazard. - Name the `-1` sentinel used for "no active powerSaveBlocker" as `POWER_SAVE_BLOCKER_INACTIVE` with a comment explaining why `-1` is a safe choice (Electron's id is always non-negative). - Drop defensive `unref?.()` on `NodeJS.Timeout` — `unref()` is guaranteed on every Node timer we handle here, so the optional-chain reads as if the method might be missing when it can't be.
018e530 to
dc1f8bb
Compare
- Extract `shutdownTimeoutMs` into a `DEFAULT_CHILD_SHUTDOWN_TIMEOUT_MS` constant next to the other orchestrator timing defaults so all knobs live together and the magic literal `5_000` in `createManager()` is no longer a scanning hazard. - Name the `-1` sentinel used for "no active powerSaveBlocker" as `POWER_SAVE_BLOCKER_INACTIVE` with a comment explaining why `-1` is a safe choice (Electron's id is always non-negative). - Drop defensive `unref?.()` on `NodeJS.Timeout` — `unref()` is guaranteed on every Node timer we handle here, so the optional-chain reads as if the method might be missing when it can't be.
dc1f8bb to
453ffd2
Compare
The `branches: [main]` filter on `pull_request` excludes PRs whose base is a sibling branch — meaning stacked PRs (#397 -> #396, #398 -> #397, etc.) report zero checks until they're collapsed onto main. Add `pr/**` to the allow-list so each link in the stack gets the same typecheck/test signal as a top-level PR.
The `branches: [main]` filter on `pull_request` excludes PRs whose base is a sibling branch — meaning stacked PRs (#397 -> #396, #398 -> #397, etc.) report zero checks until they're collapsed onto main. Add `pr/**` to the allow-list so each link in the stack gets the same typecheck/test signal as a top-level PR.
The `branches: [main]` filter on `pull_request` excludes PRs whose base is a sibling branch — meaning stacked PRs (#397 -> #396, #398 -> #397, etc.) report zero checks until they're collapsed onto main. Add `pr/**` to the allow-list so each link in the stack gets the same typecheck/test signal as a top-level PR.
The `branches: [main]` filter on `pull_request` excludes PRs whose base is a sibling branch — meaning stacked PRs (#397 -> #396, #398 -> #397, etc.) report zero checks until they're collapsed onto main. Add `pr/**` to the allow-list so each link in the stack gets the same typecheck/test signal as a top-level PR.
The CI filter that lets PRs targeting stacked-PR base branches actually run checks belongs upstack with the rest of the desktop tooling — it's unrelated to window management primitives, so keeping it in this review noise. Each downstream branch (#397, #398, #373) carries its own copy of the same one-line fix, so the stacked PRs continue to get CI runs from their own workflow files; the filter still lands on main when the stack merges, just one PR later.
31ab548 to
d3e2686
Compare
Reviewer pointed out that `git diff --check origin/main..origin/pr/427` still flags trailing blank lines in: - packages/desktop/src/window-state.ts:179 - packages/desktop/test/window-state.test.ts:590 These two files are introduced in #396 and were already fixed there as commit d932861. But this branch (#427) is stacked on `pr/desktop-7-packaging` (= OLD #396 + #397 + #398 + #373), so its tree contains the pre-fix versions and the diff vs main shows them as new-file additions with trailing blanks. Mirror the same fix here so the draft's diff is clean against main *now*, not just post-stack-merge. After the stack lands, main will already be clean (from #396's d932861) so this commit's effect on the merge is a no-op (both sides remove the same byte → identical content → clean). No semantic change.
The base branch was changed.
Root package.json / package-lock.json / CHANGELOG.md restored to match origin/main so this PR's tree no longer reverts main's release-version bumps when squash-merged. The desktop package owns its own packages/desktop/package.json; the root manifest does not need to change in this stack.
…ative-stack conflicts After #396 was squash-merged into main as 62f4095, the remaining stacked branches diverged from main on a handful of files: git's 3-way merge couldn't auto-resolve them because the branches' merge base predates #396's individual commits. Resolution: take 'ours' (the cumulative stack tip) for the contested files since the stack tip represents the authors' and reviewers' intended final state, layered with later refactors: packages/desktop/src/window-factory.ts (add/add) packages/desktop/src/window-registry.ts (add/add) packages/desktop/src/window-state.ts (add/add) packages/desktop/test/window-state.test.ts (add/add) packages/desktop/package.json (content - stack is a strict superset of main's #396 snapshot, adds electron build pipeline + node-pty dep) packages/desktop/package-lock.json (content - lock for the expanded dependency set above) Removed: packages/desktop/test/window-factory.test.ts (added by squashed #396, dropped by the later stack work; matches stack tip) The merge picks up all main-side changes that arrived after #396 (web-ui/, src/cline-sdk, scripts/, etc.) untouched.
Folds in the EOF cleanups originally proposed in #427 (also authored by @johnwschoi). 6 files, 8 trailing blank lines removed: packages/desktop/scripts/launch-electron.mjs packages/desktop/src/window-state.ts packages/desktop/test/main.test.ts packages/desktop/test/protocol-handler.test.ts packages/desktop/test/runtime-orchestrator.test.ts packages/desktop/test/window-state.test.ts These trip 'git diff --check' and would be flagged by biome on touch. Folded into this PR rather than landing #427 separately since the consolidated stack is the only path to main now.
1. Drop dead `runtimeUrl` from `WindowRegistry.CreateWindowOptions`. The field was declared on `CreateWindowOptions` and passed by `WindowFactory.create`, but never read inside `WindowRegistry` — the actual `loadURL` happens in `WindowFactory` itself. Pure dead API surface. 2. Restore `pickRecoveryUrl` + `lastUrl` capture in renderer crash recovery (regressed during the merge of main into the stack tip — we took 'ours' for window-factory.ts, but the late #396 fix-up that added pickRecoveryUrl hadn't propagated up the stack). Without this, recovery loads the bare runtime URL and silently drops whichever project/path the user was on. Restored with its 8 unit tests in packages/desktop/test/window-factory.test.ts. 3. electron-builder.yml: also exclude `dist/**/*.map`. Matches the existing `!cli/**/*.map` rule so main/preload sourcemaps don't ship in the dmg. Re-raised by review on the consolidated #397 (1c4e7b1).
ab2f219 to
34fa889
Compare
The bundled CLI subprocess crashed at startup in packaged builds with
'SyntaxError: Cannot use import statement outside a module'. cli.js is
ESM (esbuild emits 'import { createRequire } from "node:module"') but
inside the app it lives at app.asar.unpacked/cli/cli.js. Node's nearest
package.json walk-up from that directory cannot see the desktop
package.json inside the sibling app.asar archive, so Node defaulted to
CJS and choked on the first import statement. Only reproduces in
packaged DMGs, never in npm run dev.
Fix: stage-cli.mjs now writes {"type":"module"} next to the staged
cli.js so Node finds it immediately and treats the file as ESM.
Also includes the rest of the desktop-6b orchestrator fixes:
- before-quit calls preventDefault + awaits orchestrator.shutdown()
unconditionally (no isOwned() gate, which was false during mid-spawn
start() and orphaned the child)
- will-quit reduced to best-effort dispose() since Electron does not
await its handlers
- bin/kanban + kanban.cmd shims prefer ELECTRON_RUN_AS_NODE over system
node so launches don't depend on the user having node on PATH
- web-ui index.html includes x-kanban-runtime marker; orchestrator
checkHealth verifies it before attaching to an existing runtime
- shell.openExternal calls now have .catch handlers
- Startup failures now show disconnected screen instead of crashing
- Regression tests cover the structural and behavioral guarantees
AGENTS.md gains a tribal-knowledge note documenting the ESM-staging
gotcha so the next agent doesn't repeat the smoke-test debug loop.
…as identification The `x-kanban-runtime` meta tag added in 419c4ea was a P2-defense that ended up doing zero meaningful work over the title check while imposing a real cost: it silently broke attach-mode for every existing Kanban CLI in the wild (the marker only existed on this PR branch, so any globally-linked install pre-dating today rendered as 'Runtime Disconnected'). The fallback in 8ea1113 papered over it, but kept two pieces of code that have to stay in sync (producer in web-ui, consumer in desktop) for no real gain. This commit replaces the marker contract with a simpler one: grep the response body for `<title>Kanban</title>`. Properties: - Equivalent security: a foreign service still has to embed the exact title string in its 200 response on the configured runtime port to be mistaken for Kanban — vanishingly unlikely. - More stable: the title is a natural product requirement (browser tab label) so a future refactor is unlikely to drop it. The marker, by contrast, was invisible to users and easy to delete accidentally. - Zero coordination: no second place to keep in sync. Producer is just the same `<title>` element web-ui has always had. - Backward-compatible by construction: every Kanban CLI in history serves `<title>Kanban</title>`, so users with stale globally- linked installs keep working without re-linking. Removes the marker meta tag from web-ui/index.html, replaces KANBAN_RUNTIME_MARKER + KANBAN_RUNTIME_LEGACY_TITLE constants with a single KANBAN_RUNTIME_TITLE, simplifies checkHealth to one OR-less includes() call, and consolidates the test suite around the title contract.
34fa889 to
cdaaf26
Compare
Pure cleanup, no behavior change.
Dead-code removal (identified by auditing every export in PR for
external usage):
- packages/desktop/src/index.ts: empty 'export {}' barrel that no file
imports. package.json has 'main': 'dist/main.js', not index.js.
tsc was happily compiling it via src/**/*.ts and shipping ~600
bytes (.js + .d.ts + maps) into every Kanban.app.
- protocol-handler.ts: 'export interface ParsedProtocolUrl' demoted
to internal — only used as the return type of parseProtocolUrl().
- app-menu.ts: 'export interface AppMenuOptions' demoted — only used
as the AppMenu constructor parameter type.
- runtime-orchestrator.ts: 'export interface RuntimeOrchestratorOptions'
demoted — only used as the RuntimeOrchestrator constructor param.
- window-registry.ts: drop a stray blank line inside '} else {↵↵'
introduced by an earlier edit.
Review-feedback comment cleanup (no logic change, just trimming
multi-paragraph review-session narrative down to the essential
context that survives PR-merge):
- .github/workflows/test.yml: drop the stray blank line inside the
macOS Setup-Python step that broke YAML 'with:' association.
- runtime-orchestrator.ts: collapse the 26-line KANBAN_RUNTIME_TITLE
docstring (rationale, collision-risk discussion, test-parity note)
into a 4-line statement of the contract; collapse the 11-line
comment in checkHealth to a single 'see KANBAN_RUNTIME_TITLE' ref.
- runtime-orchestrator.test.ts: replace 'Reviewer-flagged race' /
'Reviewer P2:' framing with 'Regression:' framing across the four
affected test bodies; trim the 21-line setUrl-emit-contract preamble
down to a 5-line summary.
- cli-shim.test.ts: trim the 9-line 'classic failure modes' /
'history has shown' preamble to a 2-line scope statement.
239 desktop tests still pass; typecheck and lint clean.
cdaaf26 to
6e465af
Compare
What
Lands the remaining content of the desktop stack ([4b/5] runtime orchestrator, [4c/5] main wiring + OAuth/protocol, [5/5] electron-builder packaging) as a single squash-merge to avoid cascading review dismissals from sequential base-branch changes.
Why this PR ate the rest of the stack
#396 ([4a/5] window primitives) merged successfully as 62f4095. The original plan was to merge #397 → #398 → #373 sequentially. But every base change in a stacked workflow dismisses approvals on this repo, so a 4-PR sequential merge would have cost 3 separate re-review cycles, AND each step inherited add/add conflicts because the squashed parent on main couldn't auto-merge against the cumulative-history branches below.
Instead we consolidated #397's branch (
pr/desktop-6b-orchestrator) to contain the full stack tip + a merge of current main, so a single re-review unblocks everything. #398 and #373 will auto-close when this lands.Branch history
76fc9f3d— rootpackage.json/package-lock.json/ CHANGELOG kept matching main (root manifest doesn't need to change in a stack scoped topackages/desktop/; was reverting main's 0.1.64/65/66 release bumps)e17c0ba1— merge of origin/main into the stack tip, resolving 6 conflicts:packages/desktop/src/window-{factory,registry,state}.ts,packages/desktop/test/window-state.test.ts) → took stack's later-refactored versionspackages/desktop/package.json,packages/desktop/package-lock.json) → stack is a strict superset of main's [4a/5] feat(desktop): window management primitives #396 snapshot; took stackpackages/desktop/test/window-factory.test.tsto match stack tip's intent (file added by squashed [4a/5] feat(desktop): window management primitives #396 but dropped by later stack work)Diff vs main
29 files. 27 in
packages/desktop/. Non-desktop changes:.gitignore: +1 line forpackages/desktop/cli.github/workflows/test.yml: macOS-onlyactions/setup-python@v5step for node-gyp/distutils on macos-latestZero changes to
web-ui/,src/,test/,scripts/.Prior approvals (now archived as audit trail on #398/#373)
[4c/5]was approved by @maxpaulus43 (commit fe589ac) and @robinnewhouse (commit fe589ac)[5/5]was approved by @maxpaulus43 (commits bf1de07, 816b64a)The content of those PRs lives in this branch's history (
c122fb9f..76fc9f3dcarries the full stack chain).cc @maxpaulus43 @robinnewhouse for re-approval — same content as previously approved on #398 and #373, plus the merge-with-main resolution above.
Update: also folds in #427
Commit
1c4e7b1astrips trailing blank lines at EOF in 6 files (originally proposed as #427, also by @johnwschoi). #427 has been closed and points back here. 8 line deletions:packages/desktop/scripts/launch-electron.mjspackages/desktop/src/window-state.tspackages/desktop/test/main.test.tspackages/desktop/test/protocol-handler.test.tspackages/desktop/test/runtime-orchestrator.test.tspackages/desktop/test/window-state.test.ts