root/fusion
1
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

fix: harden onboarding/git/desktop batch per multi-agent review findings

Browse Source 
- uninstaller: taskkill only the first, digits-only postmaster.pid line
  (the for /f loop ran taskkill on the port/epoch lines — potential
  unrelated-process kill)
- git-missing dialogs use new ConfirmOptions.alwaysAsk so global
  skip-confirmations cannot silently pick an unseen choice
- Windows quit prompt: embedded-local runtimes only, skipped during OS
  session end (sync dialog blocked Windows shutdown)
- 'leave it running' detaches the embedded lifecycle (disarms its
  process shutdown hook) so Electron exit cannot kill the postmaster
  the operator chose to keep (new detachKeepingEmbedded)
- wizard: ref-based double-submit guard around the async git preflight
- clone route: ENOENT invalidate-and-retry matching runGitCommand
- openExternalUrl: drop the async window.open fallback (always
  popup-blocked); log bridge failures instead
- DirectoryPicker: close the panel when listing the created folder
  fails so Select cannot re-commit the parent
- git status probe bounded to two spawns (PATH + first candidate)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
gsxdsm
2026-07-18 00:33:13 -07:00
parent 8da52767b6
commit 68ce5c31da
13 changed files with 236 additions and 22 deletions
Download Patch File Download Diff File Expand all files Collapse all files

7
.changeset/review-fixes-onboarding-desktop-batch.md Normal file
View File

@@ -0,0 +1,7 @@
---
"@runfusion/fusion": patch
---
summary: Hardening pass over the onboarding, git-preflight, and Windows Postgres lifecycle features from this cycle's review.
category: fix
dev: "Uninstaller kills only the first (numeric) postmaster.pid line; git-missing dialogs render even with skip-confirmations (new ConfirmOptions.alwaysAsk); quit prompt only for embedded-local runtimes and skipped during OS session end; 'leave it running' now disarms the embedded lifecycle's process shutdown hook (detachKeepingEmbedded); wizard double-submit guard; clone route ENOENT invalidate-and-retry; openExternalUrl drops the always-popup-blocked async window.open fallback; DirectoryPicker closes the panel if listing the created folder fails; git status probe bounded to two spawns."

10
packages/core/src/git-cli-status.ts
View File

@@ -78,8 +78,16 @@ export async function probeGitCliStatus(options: ProbeGitCliStatusOptions = {}):
const first = await attemptGitVersion("git", timeoutMs);
if (typeof first === "object") return first.status;
if (first === "enoent") {
/*
FNXC:Onboarding 2026-07-18-06:00:
Review finding: probing EVERY existing candidate could take (1+N) x
timeoutMs on hosts littered with leftover git installs. Keep the probe
bounded at two spawns: PATH plus the FIRST existing well-known location
(if that one exists but cannot run, git is broken regardless).
*/
const candidates = options.fallbackGitPaths ?? wellKnownGitBinaryPaths().filter((p) => existsSync(p));
for (const candidate of candidates) {
const candidate = candidates[0];
if (candidate !== undefined) {
const attempt = await attemptGitVersion(candidate, timeoutMs);
if (typeof attempt === "object") return attempt.status;
}

17
packages/core/src/postgres/embedded-lifecycle.ts
View File

@@ -1605,6 +1605,23 @@ export class EmbeddedPostgresLifecycle {
* After stop, the data directory is preserved (persistent), so a subsequent
* `start()` reuses it.
*/
/*
FNXC:DesktopClosePolicy 2026-07-18-06:00:
Operator chose "leave the embedded PostgreSQL running" at desktop quit.
Review finding: skipping only the stop call left this lifecycle's
process-level shutdown hook (SIGTERM/SIGINT/beforeExit -> stop()) armed, so
Electron teardown killed the postmaster anyway. Detach = disarm the hook and
forget the process WITHOUT stopping it; a later stop() becomes a no-op.
*/
detachWithoutStop(): void {
this.uninstallShutdownHook();
this.pg = null;
this.nonAdminHandle = null;
this.running = false;
this.ownsProcess = false;
runningInstances.delete(this.options.dataDir);
}
async stop(): Promise<void> {
this.uninstallShutdownHook();

28
packages/core/src/postgres/startup-factory.ts
View File

@@ -183,6 +183,14 @@ export interface BackendBootResult {
* process if one was started. Best-effort; errors are logged, not thrown.
*/
shutdown(): Promise<void>;
/**
* FNXC:DesktopClosePolicy 2026-07-18-06:00:
* Close the TaskStore/pools and release the runtime lease but LEAVE an
* embedded postmaster running (disarming its process shutdown hook), for the
* desktop "leave PostgreSQL running" quit choice. No-op difference from
* shutdown() on external backends.
*/
detachKeepingEmbedded(): Promise<void>;
}
/** PostgreSQL resources used by CentralCore before a project TaskStore exists. */
@@ -1091,5 +1099,25 @@ export async function createTaskStoreForBackend(
}
}
},
async detachKeepingEmbedded() {
try {
await taskStore.close();
} catch (err) {
log.warn(`startup-factory: TaskStore.close() failed during detach: ${
err instanceof Error ? err.message : String(err)
}`);
}
if (shutdownEmbedded) {
try {
(shutdownEmbedded as unknown as { detachWithoutStop?: () => void }).detachWithoutStop?.();
if (embeddedRuntimeLease) releaseEmbeddedRuntimeLease(embeddedRuntimeLease);
} catch (err) {
log.warn(`startup-factory: embedded PostgreSQL detach failed: ${
err instanceof Error ? err.message : String(err)
}`);
}
}
},
};
}

9
packages/dashboard/app/components/DirectoryPicker.tsx
View File

@@ -155,6 +155,15 @@ export function DirectoryPicker({ value, onChange, placeholder, onInputKeyDown,
if (selectCreatedDirectory) {
onChange(result.path);
await fetchEntries(result.path, browser.showHidden);
/*
FNXC:DirectoryPicker 2026-07-18-06:00:
Review finding: if listing the just-created folder fails, the panel
silently stays on the PARENT while the input shows the child — and the
footer Select would re-commit the parent (the exact bug this feature
fixed). Close the panel on that error; the input already holds the
correct created path.
*/
setBrowser((prev) => (prev.error ? { ...prev, isOpen: false, error: null } : prev));
} else {
// Refresh entries to show the new folder
await fetchEntries(browser.currentPath, browser.showHidden);

29
packages/dashboard/app/components/SetupWizardModal.tsx
View File

@@ -151,6 +151,8 @@ export function SetupWizardModal({
}, [state.agentError]);
const detectWorkspaceRequestId = useRef(0);
// FNXC:ProjectSetup 2026-07-18-06:00: same-render double-clicks see stale state.isRegistering; the ref closes that window.
const registerInFlightRef = useRef(false);
const handlePathChange = useCallback((path: string) => {
setState((prev) => {
@@ -213,6 +215,20 @@ export function SetupWizardModal({
if (!trimmedPath || !trimmedName) return;
if (state.manualMode === "clone" && !trimmedCloneUrl) return;
if (registerInFlightRef.current || state.isRegistering) return;
registerInFlightRef.current = true;
/*
FNXC:ProjectSetup 2026-07-18-06:00:
isRegistering is set BEFORE the async git probe/dialog below (review
finding: setting it only after the awaits left a double-click window that
fired two concurrent registrations), and cleared on every abort path.
*/
setState((prev) => ({ ...prev, isRegistering: true, error: null }));
const abortRegistration = () => {
registerInFlightRef.current = false;
setState((prev) => ({ ...prev, isRegistering: false }));
};
/*
FNXC:ProjectSetup 2026-07-18-04:30:
@@ -235,8 +251,10 @@ export function SetupWizardModal({
),
confirmLabel: t("setup.gitMissingOpenDownloads", "Open Git downloads"),
cancelLabel: t("setup.cancel", "Cancel"),
alwaysAsk: true,
});
if (choice === "primary") openExternalUrl(gitCli.installUrl ?? "https://git-scm.com/downloads");
abortRegistration();
return;
}
const choice = await confirmWithChoice({
@@ -248,20 +266,23 @@ export function SetupWizardModal({
confirmLabel: t("setup.gitMissingCreateAnyway", "Create anyway without Git"),
tertiaryLabel: t("setup.gitMissingOpenDownloads", "Open Git downloads"),
cancelLabel: t("setup.cancel", "Cancel"),
alwaysAsk: true,
});
if (choice === "tertiary") {
openExternalUrl(gitCli.installUrl ?? "https://git-scm.com/downloads");
abortRegistration();
return;
}
if (choice !== "primary") {
abortRegistration();
return;
}
if (choice !== "primary") return;
skipGitInit = true;
}
} catch {
// Probe failure must not block registration.
}
setState((prev) => ({ ...prev, isRegistering: true, error: null }));
try {
const input: ProjectCreateInput = {
name: trimmedName,
@@ -298,6 +319,8 @@ export function SetupWizardModal({
isRegistering: false,
error: err instanceof Error ? err.message : "Failed to register project",
}));
} finally {
registerInFlightRef.current = false;
}
}, [includeAgentStep, onProjectRegistered, state.manualPath, state.manualName, state.manualCloneUrl, state.manualMode, state.manualIsolationMode, state.manualNodeId, state.workspaceMode, state.manualTaskPrefix, confirmWithChoice, t]);

11
packages/dashboard/app/hooks/useConfirm.ts
View File

@@ -6,6 +6,15 @@ import { ConfirmDialog } from "../components/ConfirmDialog";
export interface ConfirmOptions {
title: string;
message: string;
/*
FNXC:ConfirmDialogs 2026-07-18-06:00:
Dialogs that GATE an action on an informed choice (e.g. "git is missing —
create without a repo?") must render even when the operator globally skips
critical-action confirmations: auto-resolving "primary" would silently pick
an option the operator never saw (review finding: skip-confirmations turned
the git-missing clone dialog into an endless silent browser-tab opener).
*/
alwaysAsk?: boolean;
confirmLabel?: string;
cancelLabel?: string;
danger?: boolean;
@@ -59,7 +68,7 @@ export function ConfirmDialogProvider({
FNXC:ConfirmDialogs 2026-07-16-05:30:
Operators who globally skip critical-action confirmations must receive the same primary/default result as clicking the dialog's primary button. Never invent a different outcome or enqueue a hidden dialog; checkbox prompts retain their configured default value.
*/
if (skipConfirmationsRef.current) {
if (skipConfirmationsRef.current && !options.alwaysAsk) {
return Promise.resolve({
choice: "primary" as const,
checkboxValue: options.checkbox?.defaultChecked ?? false,

12
packages/dashboard/app/utils/__tests__/open-external.test.ts
View File

@@ -29,16 +29,24 @@ describe("openExternalUrl", () => {
expect(windowOpen).not.toHaveBeenCalled();
});
it("falls back to window.open when the bridge declines the URL", async () => {
/*
FNXC:DesktopOAuth 2026-07-18-06:00:
Review finding: a window.open fallback from the async continuation runs
without user activation and is always popup-blocked — the desktop path must
NOT pretend it helps. A declined/failed bridge is logged, never window.open'd.
*/
it("does not window.open from the async continuation when the bridge declines", async () => {
const openExternal = vi.fn().mockResolvedValue(false);
w.fusionAPI = { openExternal };
const windowOpen = vi.spyOn(window, "open").mockReturnValue(null);
const consoleError = vi.spyOn(console, "error").mockImplementation(() => {});
openExternalUrl("https://example.com/auth");
await Promise.resolve();
await Promise.resolve();
expect(windowOpen).toHaveBeenCalledWith("https://example.com/auth", "_blank");
expect(windowOpen).not.toHaveBeenCalled();
expect(consoleError).toHaveBeenCalled();
});
it("uses window.open when no desktop bridge exists", () => {

14
packages/dashboard/app/utils/open-external.ts
View File

@@ -17,14 +17,22 @@ function desktopShellApi(): DesktopShellApi | undefined {
return w.fusionAPI ?? w.electronAPI;
}
/*
FNXC:DesktopOAuth 2026-07-18-06:00:
Review finding: the old "fall back to window.open when the IPC declines" ran
window.open from an async continuation — exactly the activation-less context
this module exists to avoid, so the fallback was always popup-blocked. On
desktop the IPC is the ONLY viable opener; a failure is logged instead of
pretending a blocked fallback helped.
*/
/** Open a URL in the user's browser: desktop IPC when available, window.open otherwise. */
export function openExternalUrl(url: string): void {
const api = desktopShellApi();
if (typeof api?.openExternal === "function") {
void api.openExternal(url).then((opened) => {
if (!opened) window.open(url, "_blank");
}).catch(() => {
window.open(url, "_blank");
if (!opened) console.error(`openExternalUrl: desktop shell declined to open ${url}`);
}).catch((error: unknown) => {
console.error(`openExternalUrl: desktop shell failed to open ${url}`, error);
});
return;
}

29
packages/dashboard/src/routes/register-project-routes.ts
View File

@@ -1,6 +1,8 @@
import * as fsPromises from "node:fs/promises";
import { dirname, isAbsolute, join } from "node:path";
import {
invalidateGitBinaryCache,
isSpawnGitEnoent,
resolveGitBinary,
countRunningAgentTasks,
ensureMemoryFileWithBackend,
@@ -382,11 +384,28 @@ export const registerProjectRoutes: ApiRouteRegistrar = (ctx) => {
}
try {
await execFileAsync(await resolveGitBinary(), ["clone", cloneSource, normalizedPath], {
timeout: 90_000,
maxBuffer: 10 * 1024 * 1024,
encoding: "utf-8",
});
/*
FNXC:ProjectSetup 2026-07-18-06:00:
Review finding: match runGitCommand's ENOENT invalidate-and-retry so
a stale cached absolute git path (moved/uninstalled mid-session)
re-resolves once instead of failing every clone until restart.
*/
const runClone = (binary: string) =>
execFileAsync(binary, ["clone", cloneSource, normalizedPath], {
timeout: 90_000,
maxBuffer: 10 * 1024 * 1024,
encoding: "utf-8",
});
const cloneGit = await resolveGitBinary();
try {
await runClone(cloneGit);
} catch (firstError) {
if (!isSpawnGitEnoent(firstError)) throw firstError;
invalidateGitBinaryCache();
const retryGit = await resolveGitBinary();
if (retryGit === cloneGit) throw firstError;
await runClone(retryGit);
}
} catch (cloneError) {
if (destinationCreatedForClone) {
try {

48
packages/desktop/build/uninstaller.nsh
View File

@@ -6,11 +6,53 @@
; uninstalls ask before deleting it, silent uninstalls always keep it, and
; auto-update reinstalls (${isUpdated}) touch nothing so updates never kill a
; running server or prompt.
;
; FNXC:WindowsDesktopPackaging 2026-07-18-06:00:
; Review finding: a for /f loop over postmaster.pid ran taskkill on EVERY line
; (line 4 is the TCP port — a plausible PID of some unrelated process). Read
; ONLY the first line, trim CR/LF, and require it to be purely numeric before
; killing anything.
!macro customUnInstall
${ifNot} ${isUpdated}
; Stop the postmaster recorded in postmaster.pid (best effort; the PID is
; the file's first line). usebackq tolerates spaces in the profile path.
nsExec::ExecToLog 'cmd /c for /f "usebackq" %i in ("$PROFILE\.fusion\embedded-postgres\default\postmaster.pid") do taskkill /PID %i /F /T'
; Read the postmaster PID: first line of postmaster.pid, digits only.
ClearErrors
FileOpen $0 "$PROFILE\.fusion\embedded-postgres\default\postmaster.pid" r
${ifNot} ${Errors}
FileRead $0 $1
FileClose $0
; Trim trailing CR/LF.
loop_trim:
StrCpy $2 $1 1 -1
${if} $2 == "$\r"
StrCpy $1 $1 -1
Goto loop_trim
${endif}
${if} $2 == "$\n"
StrCpy $1 $1 -1
Goto loop_trim
${endif}
; Digits-only guard: reject empty or any non-numeric character.
StrCpy $3 0
${if} $1 == ""
Goto skip_kill
${endif}
digit_check:
StrCpy $2 $1 1 $3
${if} $2 == ""
Goto do_kill
${endif}
${if} $2 < "0"
Goto skip_kill
${endif}
${if} $2 > "9"
Goto skip_kill
${endif}
IntOp $3 $3 + 1
Goto digit_check
do_kill:
nsExec::ExecToLog 'taskkill /PID $1 /F /T'
skip_kill:
${endIf}
RMDir /r "$PROFILE\.fusion\embedded-postgres\runtime-bin"
IfSilent +3 0
MessageBox MB_YESNO|MB_ICONQUESTION "Also delete the embedded PostgreSQL database (all local Fusion data) at $PROFILE\.fusion\embedded-postgres?" IDNO +2

21
packages/desktop/src/local-runtime.ts
View File

@@ -134,6 +134,9 @@ async function createStoreDefault(
// Attach the backend shutdown so LocalRuntimeManager can invoke it on stop.
(store as TaskStoreLike & { __backendShutdown?: () => Promise<void> }).__backendShutdown =
backendBoot.shutdown;
// FNXC:DesktopClosePolicy 2026-07-18-06:00: detach variant for the "leave PostgreSQL running" quit answer.
(store as TaskStoreLike & { __backendDetach?: () => Promise<void> }).__backendDetach =
backendBoot.detachKeepingEmbedded;
return store;
}
@@ -602,9 +605,21 @@ export class LocalRuntimeManager {
running for other Fusion processes. Default (false) preserves the full
teardown for programmatic restarts and every non-prompted path.
*/
const backendShutdown = (runtime.store as TaskStoreLike & { __backendShutdown?: () => Promise<void> }).__backendShutdown;
if (backendShutdown && !options.keepEmbeddedPostgres) {
await backendShutdown().catch(() => undefined);
const storeWithBackend = runtime.store as TaskStoreLike & {
__backendShutdown?: () => Promise<void>;
__backendDetach?: () => Promise<void>;
};
if (options.keepEmbeddedPostgres && storeWithBackend.__backendDetach) {
/*
FNXC:DesktopClosePolicy 2026-07-18-06:00:
Review finding: skipping the backend shutdown alone left the embedded
lifecycle's process shutdown hook armed, so Electron exit stopped the
postmaster despite the operator's "leave it running" answer. Detach
closes pools and DISARMS that hook without stopping the server.
*/
await storeWithBackend.__backendDetach().catch(() => undefined);
} else if (storeWithBackend.__backendShutdown) {
await storeWithBackend.__backendShutdown().catch(() => undefined);
} else {
runtime.store.close();
}

23
packages/desktop/src/main.ts
View File

@@ -137,6 +137,13 @@ false (default) = full stop including the embedded PostgreSQL cluster.
*/
let keepEmbeddedPostgresOnQuit = false;
/*
FNXC:DesktopClosePolicy 2026-07-18-06:00:
Set by Electron's win32 "session-end" event: the OS is logging off or shutting
down, so the close prompt must be skipped (see close handler).
*/
let osSessionEnding = false;
async function resetLaunchModeAndReload(window: BrowserWindow): Promise<void> {
try {
const settings = await readShellSettings();
@@ -255,7 +262,17 @@ export function createMainWindow(state?: WindowState, launchTargetUrl?: string):
event cannot await, and the answer must exist before before-quit tears the
runtime down.
*/
if (localRuntimeManager?.getStatus().state === "running") {
/*
FNXC:DesktopClosePolicy 2026-07-18-06:00:
Review findings: (a) only the EMBEDDED local runtime is ours to stop —
an attached external `fn serve` runtime ignores both answers, so never
prompt for it; (b) never prompt during OS session end (logoff/shutdown):
the sync dialog would block Windows shutdown until the process is
force-killed, skipping all teardown. Electron's app "session-end" event
flags that case; the default (full stop) then applies.
*/
const runtimeStatus = localRuntimeManager?.getStatus();
if (!osSessionEnding && runtimeStatus?.state === "running" && runtimeStatus.source === "embedded-local") {
const choice = dialog.showMessageBoxSync(window, {
type: "question",
title: "Fusion",
@@ -499,6 +516,10 @@ export function run(): void {
}
});
(app as unknown as { on(event: "session-end", listener: () => void): void }).on("session-end", () => {
osSessionEnding = true;
});
app.on("before-quit", () => {
appWithQuitFlag.isQuitting = true;