chore(desktop): auto-restart host-service on bundle change in dev#3461
Conversation
Watches the built host-service.js in NODE_ENV=development and restarts running instances via the coordinator when electron-vite rewrites the bundle. Fast edit→reload loop for packages/host-service and src/main/host-service without restarting Electron. Not true HMR — in-memory host-service state (PTYs, watchers, chat streams) is torn down on each reload.
📝 WalkthroughWalkthroughAdds a development-only hot-reload feature: Electron main loads an auth token and API URL and enables a host-service coordinator dev watcher that detects host-service bundle changes, debounces and waits for stabilization, fetches updated spawn config, and restarts running host-service instances. Changes
Sequence Diagram(s)sequenceDiagram
participant Main as Electron Main
participant Auth as Auth Loader
participant Env as mainEnv
participant HSC as HostServiceCoordinator
participant FS as File System
participant Config as Config Provider
Main->>Auth: call loadToken()
Auth-->>Main: token | null
Main->>Env: read NEXT_PUBLIC_API_URL
Main->>HSC: enableDevReload(configProvider, {authToken, cloudApiUrl})
HSC->>FS: fs.watch(bundleDir)
Note over FS: watching host-service bundle
FS->>HSC: change event
HSC->>HSC: debounce + wait for stable size
HSC->>Config: call configProvider()
Config-->>HSC: SpawnConfig | null
HSC->>HSC: restartAll(SpawnConfig)
HSC-->>Main: teardown function
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR adds a dev-only hot-rebuild loop for the host-service in the desktop Electron app. In
Confidence Score: 5/5Safe to merge — dev-only feature with correct production gating; both findings are non-blocking P2 style suggestions. The feature is gated strictly on No files require special attention; both comments are on Important Files Changed
Sequence DiagramsequenceDiagram
participant EV as electron-vite
participant FS as fs.watch (scriptDir)
participant TR as trigger() / debounce
participant CP as configProvider (loadToken)
participant HC as HostServiceCoordinator
participant HS as host-service process(es)
EV->>FS: rewrites host-service.js
FS->>TR: onChange event (filename=host-service.js)
TR->>TR: clearTimeout(prev debounce)
TR->>TR: setTimeout 250ms
Note over TR: debounce window
TR->>TR: debounce fires
TR->>HC: getActiveOrganizationIds()
alt no running instances
HC-->>TR: [] → no-op
else instances running
HC-->>TR: [orgId, ...]
TR->>CP: configProvider()
CP->>CP: loadToken()
alt user not signed in
CP-->>TR: null → no-op
else signed in
CP-->>TR: { authToken, cloudApiUrl }
TR->>HC: restartAll(config)
HC->>HS: stop(orgId) → SIGTERM
HC->>HS: spawn(orgId, config)
HS-->>HC: healthy (port, secret)
HC-->>TR: done
end
end
Reviews (1): Last reviewed commit: "chore(desktop): auto-restart host-servic..." | Re-trigger Greptile |
There was a problem hiding this comment.
1 issue found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/main/lib/host-service-coordinator.ts">
<violation number="1" location="apps/desktop/src/main/lib/host-service-coordinator.ts:252">
P3: Race condition: `reloading = true` is set after the `await configProvider()` gap, so two invocations can both pass the `if (reloading) return` check before either sets the flag. Move `reloading = true` before the first `await` to actually guard against concurrent restarts.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Rollup rewrites host-service.js via unlink+rewrite, so the fs.watch fires while the file is missing or partially written. The coordinator now polls statSync until size is non-zero and stable for 150ms (5s deadline) before calling restartAll, avoiding MODULE_NOT_FOUND on respawn.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/main/lib/host-service-coordinator.ts (1)
234-237: Repeated calls return a no-op teardown.If
enableDevReloadis called multiple times, subsequent calls return() => {}instead of the actual teardown function. This means only the first caller can properly clean up the watcher. If this method might be called from multiple places or re-invoked, consider returning the existing teardown capability or storing it for retrieval.💡 Option: return a shared teardown or log a warning
enableDevReload( configProvider: () => Promise<SpawnConfig | null>, ): () => void { - if (this.devReloadWatcher) return () => {}; + if (this.devReloadWatcher) { + console.warn("[host-service] dev reload already enabled"); + return () => {}; + }Alternatively, store the teardown function as a field and return it on repeated calls.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/host-service-coordinator.ts` around lines 234 - 237, enableDevReload currently returns a no-op when called a second time because it only checks this.devReloadWatcher and returns () => {} instead of the actual teardown, so change the logic to return the existing teardown function when a watcher already exists: capture and store the real teardown (e.g., assign to a field like this.devReloadTeardown or attach it to this.devReloadWatcher) when creating the watcher inside enableDevReload and on subsequent calls return that stored teardown instead of an empty closure; ensure the teardown clears both the watcher and the stored teardown field so repeated enable/teardown cycles remain consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/desktop/src/main/lib/host-service-coordinator.ts`:
- Around line 234-237: enableDevReload currently returns a no-op when called a
second time because it only checks this.devReloadWatcher and returns () => {}
instead of the actual teardown, so change the logic to return the existing
teardown function when a watcher already exists: capture and store the real
teardown (e.g., assign to a field like this.devReloadTeardown or attach it to
this.devReloadWatcher) when creating the watcher inside enableDevReload and on
subsequent calls return that stored teardown instead of an empty closure; ensure
the teardown clears both the watcher and the stored teardown field so repeated
enable/teardown cycles remain consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 78349523-938e-4bc7-9df6-114b7d828e3e
📒 Files selected for processing (1)
apps/desktop/src/main/lib/host-service-coordinator.ts
…perset-sh#3461) * chore(desktop): auto-restart host-service on bundle change in dev Watches the built host-service.js in NODE_ENV=development and restarts running instances via the coordinator when electron-vite rewrites the bundle. Fast edit→reload loop for packages/host-service and src/main/host-service without restarting Electron. Not true HMR — in-memory host-service state (PTYs, watchers, chat streams) is torn down on each reload. * fix(desktop): wait for host-service bundle to stabilize before reload Rollup rewrites host-service.js via unlink+rewrite, so the fs.watch fires while the file is missing or partially written. The coordinator now polls statSync until size is non-zero and stable for 150ms (5s deadline) before calling restartAll, avoiding MODULE_NOT_FOUND on respawn.
Two adjustments on top of the cherry-picked upstream commits: 1. apps/desktop/src/main/index.ts — upstream superset-sh#3461 uses getHostServiceCoordinator() at the dev-reload call site, but fork imports that symbol as getHostServiceManager to minimize diff with the quit-lifecycle code that still uses the legacy name. Rewrite the call to use the local alias so the desktop main process still builds. 2. apps/desktop/src/renderer/routes/_authenticated/settings/layout.tsx — upstream superset-sh#3466 uses [data-state="open"] to suppress Escape when a Radix overlay is open, but that selector also matches Radix Collapsible (settings/agents AgentCard uses Collapsible with data-state="open"), so expanding any card silently disabled the new Escape-to-close behavior. Narrow the selector to role-based overlay elements only (dialog, alertdialog, menu, listbox).
…ndle chore(upstream): PR1 低リスク修正5件バンドル (superset-sh#3457 superset-sh#3464 superset-sh#3462 superset-sh#3466 superset-sh#3461)
-s ours merge to record that upstream commits a3e34bf through de70163 (13 commits) are semantically already present on origin/main via the PR1-6 cherry-pick series (PRs #176, #177, #178, #179, #180, #182), plus fork-adaptation fixes layered on top. This merge target is de70163 specifically (not upstream/main) so newer upstream commits (9fff075 and later) remain visible in future behind counts. Upstream commits covered by this audit merge: - a3e34bf fix(desktop): restore cmd+click requirement for v1 terminal file links (superset-sh#3457) [PR1/#176] - 57557f8 fix(desktop): gate v2 workspace children on collection readiness (superset-sh#3464) [PR1/#176] - 4ee2e61 fix(desktop): use native clipboard for copy path in v2 sidebar (superset-sh#3462) [PR1/#176] - 87d6e93 feat(desktop): close settings with Escape key (superset-sh#3466) [PR1/#176] - 9c7f5f4 chore(desktop): auto-restart host-service on bundle change in dev (superset-sh#3461) [PR1/#176] - 93140d9 fix(mcp): accept MCP resource URL as valid OAuth audience (superset-sh#3459) [PR2/#177] - be9e000 fix(desktop): drive tray menu off events, fetch real org name (superset-sh#3458) [PR2/#177] - c5f791e feat(v2): unify workspace delete through host-service (superset-sh#3443) [PR3/#178] - 2c24d93 feat(desktop): paginated branch picker with checkout + open actions (superset-sh#3397) [PR4/#179] - 2bf1049 feat(desktop/hotkeys): v1 directional pane focus + best-effort v1 override migrator (superset-sh#3460) [PR5/#180] - 1294a7d feat(desktop/hotkeys): restore Cmd+Alt+Arrow for tab/workspace nav (superset-sh#3472) [PR5/#180] - de70163 feat(desktop): v2 review tab first pass — PR info, checks, comments (superset-sh#3463) [PR6/#182] Intentionally skipped (version bump, fork has independent versioning): - 1e23353 chore(desktop): bump version to 1.5.5 (superset-sh#3473) Fork-adaptation fixes layered on top of the cherry-picks: - PR1: host-service-coordinator alias import fix, settings Escape selector narrowing (role-based + popper wrapper), Escape close uses replace navigation - PR2: dual quit mode preservation (requestQuit "release"/"stop"), trayUpdateToken guard for stale async fetchHostInfo results - PR4: ChangesHeader.normalizeBranchName regex rewrite (lint false positive), worktree add uses fullRef for remote-tracking refs, syncTimedOut reset on pendingId change, GIT_REFS.md barrel example fix - PR5: migrate.ts re-sanitize of existing localStorage overrides (v2 marker bump intent), FOCUS_PANE_* enabled:isActive for KeepAliveWorkspaces, CATEGORY_ORDER merges Navigation (upstream) and Browser (fork) - PR6: normalizeThreadsToComments flattens all thread.comments (not just first), CommentPane overrides <a> (openUrl) and <img> (SafeImage), zero-badge suppression, pr-null comments gate Fork features verified intact (Explore agent audit of combined 36d4de4..35d95f3 range): - BROWSER_RELOAD / BROWSER_HARD_RELOAD hotkeys - dual quit mode menu in tray - v1 terminal cold-restore + retry reconnect (out of range but unaffected) - KeepAliveWorkspaces (FOCUS_PANE_* gated on isActive) - useCommandPalette + addMemoTab in v2 workspace - host-service-coordinator rename alias pattern
Summary
NODE_ENV=development, watches the builthost-service.jsbundle and restarts running host-service instances via the coordinator when electron-vite rewrites it.HostServiceCoordinator.enableDevReload(configProvider)debounces file events (250ms) and callsrestartAllwith the current auth token; no-ops when no instances are running or the user isn't signed in.main/index.tsafterdiscoverAll().Not true HMR — workspace-scoped in-memory state (PTYs, watchers, chat streams) is torn down on each reload. Edit → rebuild → respawn is close enough for iterating on
packages/host-serviceandsrc/main/host-servicewithout restarting Electron.Test plan
bun devinapps/desktop, sign in, open a workspace so a host-service instance is running.packages/host-service/src(e.g. add aconsole.loginserve.tsor a router); confirm electron-vite rebuilds and the main-process log shows[host-service] bundle changed, restarting running instances.host-service.js; confirm no reload attempt happens (no-op path).IS_DEV).Summary by cubic
Auto-restart the host-service in development when the built
host-service.jschanges, waiting for the bundle to stabilize before restart. Enables a fast edit→reload loop without restarting Electron; dev-only and not HMR—host-service in-memory state resets on each reload.HostServiceCoordinator.enableDevReload(configProvider)to watch the built bundle, wait for a stable file size (~150ms, 5s deadline), debounce events (250ms), and restart running instances with the current auth token; avoids respawn errors during Rollup’s unlink+rewrite.main/index.tsafterdiscoverAll()usingloadToken()andmainEnv.NEXT_PUBLIC_API_URL.Written for commit e6ae2c5. Summary will update on new commits.
Summary by CodeRabbit