diff --git a/.changeset/review-fixes-onboarding-desktop-batch.md b/.changeset/review-fixes-onboarding-desktop-batch.md new file mode 100644 index 0000000000..305e838fb9 --- /dev/null +++ b/.changeset/review-fixes-onboarding-desktop-batch.md @@ -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." diff --git a/packages/core/src/git-cli-status.ts b/packages/core/src/git-cli-status.ts index 3004cf95e3..0d007c174c 100644 --- a/packages/core/src/git-cli-status.ts +++ b/packages/core/src/git-cli-status.ts @@ -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; } diff --git a/packages/core/src/postgres/embedded-lifecycle.ts b/packages/core/src/postgres/embedded-lifecycle.ts index 8d325e2f69..0a08844ed8 100644 --- a/packages/core/src/postgres/embedded-lifecycle.ts +++ b/packages/core/src/postgres/embedded-lifecycle.ts @@ -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 { this.uninstallShutdownHook(); diff --git a/packages/core/src/postgres/startup-factory.ts b/packages/core/src/postgres/startup-factory.ts index 83de84227e..67f5d1318c 100644 --- a/packages/core/src/postgres/startup-factory.ts +++ b/packages/core/src/postgres/startup-factory.ts @@ -183,6 +183,14 @@ export interface BackendBootResult { * process if one was started. Best-effort; errors are logged, not thrown. */ shutdown(): Promise; + /** + * 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; } /** 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) + }`); + } + } + }, }; } diff --git a/packages/dashboard/app/components/DirectoryPicker.tsx b/packages/dashboard/app/components/DirectoryPicker.tsx index 429bbc4138..d3b14362ac 100644 --- a/packages/dashboard/app/components/DirectoryPicker.tsx +++ b/packages/dashboard/app/components/DirectoryPicker.tsx @@ -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); diff --git a/packages/dashboard/app/components/SetupWizardModal.tsx b/packages/dashboard/app/components/SetupWizardModal.tsx index 09adcda150..f91c7be754 100644 --- a/packages/dashboard/app/components/SetupWizardModal.tsx +++ b/packages/dashboard/app/components/SetupWizardModal.tsx @@ -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]); diff --git a/packages/dashboard/app/hooks/useConfirm.ts b/packages/dashboard/app/hooks/useConfirm.ts index 5faba7f5c2..62173a0f4d 100644 --- a/packages/dashboard/app/hooks/useConfirm.ts +++ b/packages/dashboard/app/hooks/useConfirm.ts @@ -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, diff --git a/packages/dashboard/app/utils/__tests__/open-external.test.ts b/packages/dashboard/app/utils/__tests__/open-external.test.ts index f7e1a614be..4ed27f7ec6 100644 --- a/packages/dashboard/app/utils/__tests__/open-external.test.ts +++ b/packages/dashboard/app/utils/__tests__/open-external.test.ts @@ -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", () => { diff --git a/packages/dashboard/app/utils/open-external.ts b/packages/dashboard/app/utils/open-external.ts index af1c999d77..afcb8ea95c 100644 --- a/packages/dashboard/app/utils/open-external.ts +++ b/packages/dashboard/app/utils/open-external.ts @@ -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; } diff --git a/packages/dashboard/src/routes/register-project-routes.ts b/packages/dashboard/src/routes/register-project-routes.ts index de753c1b28..04980b070f 100644 --- a/packages/dashboard/src/routes/register-project-routes.ts +++ b/packages/dashboard/src/routes/register-project-routes.ts @@ -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 { diff --git a/packages/desktop/build/uninstaller.nsh b/packages/desktop/build/uninstaller.nsh index 41f0c7377c..e5c395b72c 100644 --- a/packages/desktop/build/uninstaller.nsh +++ b/packages/desktop/build/uninstaller.nsh @@ -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 diff --git a/packages/desktop/src/local-runtime.ts b/packages/desktop/src/local-runtime.ts index e5ea5fb683..65285d7bf4 100644 --- a/packages/desktop/src/local-runtime.ts +++ b/packages/desktop/src/local-runtime.ts @@ -134,6 +134,9 @@ async function createStoreDefault( // Attach the backend shutdown so LocalRuntimeManager can invoke it on stop. (store as TaskStoreLike & { __backendShutdown?: () => Promise }).__backendShutdown = backendBoot.shutdown; + // FNXC:DesktopClosePolicy 2026-07-18-06:00: detach variant for the "leave PostgreSQL running" quit answer. + (store as TaskStoreLike & { __backendDetach?: () => Promise }).__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 }).__backendShutdown; - if (backendShutdown && !options.keepEmbeddedPostgres) { - await backendShutdown().catch(() => undefined); + const storeWithBackend = runtime.store as TaskStoreLike & { + __backendShutdown?: () => Promise; + __backendDetach?: () => Promise; + }; + 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(); } diff --git a/packages/desktop/src/main.ts b/packages/desktop/src/main.ts index 86ea1ea88e..d1b2b101f3 100644 --- a/packages/desktop/src/main.ts +++ b/packages/desktop/src/main.ts @@ -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 { 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;