From 56dd6eba69e82511ef8db02646f2951a329b115b Mon Sep 17 00:00:00 2001 From: skulidropek <66840575+skulidropek@users.noreply.github.com> Date: Fri, 29 May 2026 21:35:31 +0000 Subject: [PATCH 1/5] fix(web): retain prepared browser popup handle --- packages/app/src/web/open-url.ts | 2 +- .../tests/docker-git/actions-browser.test.ts | 4 +- .../tests/docker-git/actions-skiller.test.ts | 2 +- .../app/tests/docker-git/open-url.test.ts | 47 +++++++++++++++++++ 4 files changed, 51 insertions(+), 4 deletions(-) create mode 100644 packages/app/tests/docker-git/open-url.test.ts diff --git a/packages/app/src/web/open-url.ts b/packages/app/src/web/open-url.ts index 78e967b4..d4ab5582 100644 --- a/packages/app/src/web/open-url.ts +++ b/packages/app/src/web/open-url.ts @@ -20,7 +20,7 @@ export const prepareOpenUrl = (): PreparedOpenUrl => { if (typeof globalThis.open !== "function") { return blockedPreparedOpenUrl() } - const openedWindow = globalThis.open("about:blank", "_blank", "noopener") + const openedWindow = globalThis.open("about:blank", "_blank") if (openedWindow === null) { return blockedPreparedOpenUrl() } diff --git a/packages/app/tests/docker-git/actions-browser.test.ts b/packages/app/tests/docker-git/actions-browser.test.ts index 783c266f..49ea145f 100644 --- a/packages/app/tests/docker-git/actions-browser.test.ts +++ b/packages/app/tests/docker-git/actions-browser.test.ts @@ -67,7 +67,7 @@ describe("web browser actions", () => { expect(setProjectBrowser).toHaveBeenCalledWith(runningBrowser) })) - expect(openMock).toHaveBeenCalledWith("about:blank", "_blank", "noopener") + expect(openMock).toHaveBeenCalledWith("about:blank", "_blank") expect(openedWindow.location.href).toBe("/api/projects/project-1/browser/novnc") expect(setMessage).toHaveBeenLastCalledWith("Browser opened. CDP endpoint: /api/projects/project-1/browser/cdp.") })) @@ -89,7 +89,7 @@ describe("web browser actions", () => { })) expect(startProjectBrowserMock).toHaveBeenCalledWith("project-1") - expect(openMock).toHaveBeenCalledWith("about:blank", "_blank", "noopener") + expect(openMock).toHaveBeenCalledWith("about:blank", "_blank") expect(openedWindow.close).toHaveBeenCalledOnce() expect(setMessage).toHaveBeenLastCalledWith( "Browser runtime is missing. Enable Playwright MCP and start the project first." diff --git a/packages/app/tests/docker-git/actions-skiller.test.ts b/packages/app/tests/docker-git/actions-skiller.test.ts index ba23515b..44b32029 100644 --- a/packages/app/tests/docker-git/actions-skiller.test.ts +++ b/packages/app/tests/docker-git/actions-skiller.test.ts @@ -87,7 +87,7 @@ describe("web Skiller actions", () => { openSkillerApp(context) - expect(browserOpenMock).toHaveBeenCalledWith("about:blank", "_blank", "noopener") + expect(browserOpenMock).toHaveBeenCalledWith("about:blank", "_blank") expect(setMessage).toHaveBeenCalledWith("Opening Skiller...") yield* _(waitForAssertion(() => { expect(openSkillerMock).toHaveBeenCalledWith(undefined, undefined) diff --git a/packages/app/tests/docker-git/open-url.test.ts b/packages/app/tests/docker-git/open-url.test.ts new file mode 100644 index 00000000..6f7c28c9 --- /dev/null +++ b/packages/app/tests/docker-git/open-url.test.ts @@ -0,0 +1,47 @@ +import { describe, expect, it } from "@effect/vitest" +import { afterEach, vi } from "vitest" + +import { openUrl, prepareOpenUrl } from "../../src/web/open-url.js" +import { makeBrowserOpenMockWindow, stubBrowserOpen } from "./browser-open-fixture.js" + +describe("open-url helpers", () => { + afterEach(() => { + vi.unstubAllGlobals() + }) + + it("opens prepared async popups without noopener so the caller keeps the window handle", () => { + const openedWindow = makeBrowserOpenMockWindow() + const openMock = stubBrowserOpen(openedWindow) + + const prepared = prepareOpenUrl() + + expect(openMock).toHaveBeenCalledWith("about:blank", "_blank") + expect(openedWindow.opener).toBeNull() + expect(prepared.navigate("/api/projects/project-1/browser/novnc")).toBe(true) + expect(openedWindow.location.href).toBe("/api/projects/project-1/browser/novnc") + expect(openedWindow.focus).toHaveBeenCalledOnce() + + prepared.close() + + expect(openedWindow.close).toHaveBeenCalledOnce() + }) + + it("falls back to direct noopener open when the prepared popup is blocked", () => { + const openedWindow = makeBrowserOpenMockWindow() + const openMock = vi.fn((url: string) => url === "about:blank" ? null : openedWindow) + vi.stubGlobal("open", openMock) + + const prepared = prepareOpenUrl() + + expect(prepared.navigate("/api/projects/project-1/browser/novnc")).toBe(true) + expect(openMock).toHaveBeenNthCalledWith(1, "about:blank", "_blank") + expect(openMock).toHaveBeenNthCalledWith(2, "/api/projects/project-1/browser/novnc", "_blank", "noopener") + }) + + it("reports direct opens as blocked when no browser open function exists", () => { + vi.stubGlobal("open", null) + + expect(openUrl("/api/projects/project-1/browser/novnc")).toBe(false) + expect(prepareOpenUrl().navigate("/api/projects/project-1/browser/novnc")).toBe(false) + }) +}) From 858b84a68de7ee516f53097421ba1cb0bc2bc642 Mon Sep 17 00:00:00 2001 From: skulidropek <66840575+skulidropek@users.noreply.github.com> Date: Fri, 29 May 2026 23:43:25 +0000 Subject: [PATCH 2/5] fix(browser): wait for current controller and runtime --- packages/api/src/services/project-browser.ts | 30 ++++++- .../app/src/docker-git/controller-health.ts | 51 +++++++++-- .../controller-reachability-diagnostics.ts | 21 +++++ packages/app/src/docker-git/controller.ts | 39 ++++----- .../docker-git/controller-health.test.ts | 86 +++++++++++++++++++ 5 files changed, 197 insertions(+), 30 deletions(-) create mode 100644 packages/app/src/docker-git/controller-reachability-diagnostics.ts create mode 100644 packages/app/tests/docker-git/controller-health.test.ts diff --git a/packages/api/src/services/project-browser.ts b/packages/api/src/services/project-browser.ts index 4517c3bf..0f12ab4d 100644 --- a/packages/api/src/services/project-browser.ts +++ b/packages/api/src/services/project-browser.ts @@ -10,7 +10,7 @@ import type { PlatformError } from "@effect/platform/Error" import * as HttpServerError from "@effect/platform/HttpServerError" import * as HttpServerRequest from "@effect/platform/HttpServerRequest" import * as HttpServerResponse from "@effect/platform/HttpServerResponse" -import { Effect } from "effect" +import { Duration, Effect, pipe, Schedule } from "effect" import * as Stream from "effect/Stream" import type { IncomingMessage, Server as HttpServer } from "node:http" import { createConnection, type Socket } from "node:net" @@ -96,6 +96,7 @@ const cdpHostHeader = "127.0.0.1:9222" const browserActivityWriteIntervalMs = 30_000 const browserActivityWrites = new Map() const browserWebSocketCounts = new Map() +const browserRuntimeReadySchedule = Schedule.addDelay(Schedule.recurs(40), () => Duration.millis(250)) const hopByHopRequestHeaders = new Set([ "connection", @@ -248,6 +249,32 @@ const startBrowserContainer = ( ) ) +const renderBrowserRuntimeReadyProbeScript = (): string => [ + "set -eu", + `for port in ${browserNoVncPort} ${browserVncPort} ${browserCdpPort}; do`, + " timeout 1 bash -lc \" + pipe( + dockerCapture( + cwd, + ["exec", projectContainerName, "bash", "-lc", renderBrowserRuntimeReadyProbeScript()], + "docker exec browser runtime ready probe" + ), + Effect.asVoid, + Effect.retry(browserRuntimeReadySchedule), + Effect.mapError(() => + new ApiConflictError({ + message: `Browser runtime did not become ready for ${projectContainerName}.` + }) + ) + ) + const parseContainerNetworkEntries = (output: string): ReadonlyArray => output .trim() @@ -439,6 +466,7 @@ export const startProjectBrowserSession = ( const project = yield* _(getProjectItemById(projectId)) const containerName = browserContainerName(project.containerName) yield* _(startBrowserContainer(project.projectDir, project.containerName)) + yield* _(waitForBrowserRuntimeReady(project.projectDir, project.containerName)) const state = yield* _(inspectBrowserContainerState(project.projectDir, containerName)) return browserSessionFromState(projectId, containerName, state, externalOrigin) }) diff --git a/packages/app/src/docker-git/controller-health.ts b/packages/app/src/docker-git/controller-health.ts index d5574be7..31bd1119 100644 --- a/packages/app/src/docker-git/controller-health.ts +++ b/packages/app/src/docker-git/controller-health.ts @@ -64,7 +64,8 @@ const probeHealth = (apiBaseUrl: string): Effect.Effect + candidateUrls: ReadonlyArray, + expectedRevision?: string ): Effect.Effect => Effect.gen(function*(_) { if (candidateUrls.length === 0) { @@ -73,14 +74,19 @@ const findReachableHealthProbe = ( ) } + const mismatches: Array = [] for (const candidateUrl of candidateUrls) { const healthy = yield* _(probeHealth(candidateUrl).pipe(Effect.either)) - if (Either.isRight(healthy)) { + if (Either.isLeft(healthy)) { + continue + } + if (matchesExpectedRevision(healthy.right, expectedRevision)) { return healthy.right } + mismatches.push(describeRevisionMismatch(healthy.right)) } - return yield* _(Effect.fail(controllerBootstrapError("No docker-git controller endpoint responded to /health."))) + return yield* _(Effect.fail(noMatchingHealthProbeError(expectedRevision, mismatches))) }) const findReachableHealthProbeOrNull = ( @@ -93,10 +99,45 @@ const findReachableHealthProbeOrNull = ( }) ) +const matchesExpectedRevision = ( + probe: HealthProbeResult, + expectedRevision: string | undefined +): boolean => expectedRevision === undefined || probe.revision === expectedRevision + +const describeRevisionMismatch = (probe: HealthProbeResult): string => + `${probe.apiBaseUrl} revision ${probe.revision ?? "unknown"}` + +const noMatchingHealthProbeError = ( + expectedRevision: string | undefined, + mismatches: ReadonlyArray +): ControllerBootstrapError => + expectedRevision !== undefined && mismatches.length > 0 + ? controllerBootstrapError( + `No docker-git controller endpoint with revision ${expectedRevision} responded. ` + + `Reachable mismatched controllers: ${mismatches.join(", ")}.` + ) + : controllerBootstrapError("No docker-git controller endpoint responded to /health.") + export const findReachableApiBaseUrl = ( - candidateUrls: ReadonlyArray + candidateUrls: ReadonlyArray, + expectedRevision?: string ): Effect.Effect => - findReachableHealthProbe(candidateUrls).pipe(Effect.map(({ apiBaseUrl }) => apiBaseUrl)) + findReachableHealthProbe(candidateUrls, expectedRevision).pipe(Effect.map(({ apiBaseUrl }) => apiBaseUrl)) + +// CHANGE: select only controller endpoints that prove the expected source revision. +// WHY: containerized hosts can see stale controllers through host.docker.internal before the current local controller is reachable. +// QUOTE(ТЗ): "проверь сам что Open Browser кнопка работает" +// REF: user-message-2026-05-29-open-browser-e2e +// SOURCE: n/a +// FORMAT THEOREM: selected(endpoint) -> health(endpoint).revision = expectedRevision +// PURITY: SHELL +// EFFECT: FetchHttpClient health probes. +// INVARIANT: mismatched reachable controllers are rejected rather than reused. +// COMPLEXITY: O(n) health probes where n = |candidateUrls|. +export const findReachableApiBaseUrlMatchingRevision = ( + candidateUrls: ReadonlyArray, + expectedRevision: string +): Effect.Effect => findReachableApiBaseUrl(candidateUrls, expectedRevision) export const findReachableDirectHealthProbe = (options: { readonly explicitApiBaseUrl: string | undefined diff --git a/packages/app/src/docker-git/controller-reachability-diagnostics.ts b/packages/app/src/docker-git/controller-reachability-diagnostics.ts new file mode 100644 index 00000000..de8ea556 --- /dev/null +++ b/packages/app/src/docker-git/controller-reachability-diagnostics.ts @@ -0,0 +1,21 @@ +import { Effect } from "effect" + +import * as ControllerDocker from "./controller-docker.js" +import { type DockerNetworkIps, formatNetworkIps } from "./controller-reachability.js" + +export const collectReachabilityDiagnostics = ( + candidateUrls: ReadonlyArray, + currentContainerNetworks: DockerNetworkIps, + controllerNetworks: DockerNetworkIps +): Effect.Effect => + Effect.gen(function*(_) { + const publishedPorts = yield* _(ControllerDocker.inspectControllerPublishedPorts()) + + return [ + "Tried endpoints:", + ...candidateUrls.map((candidateUrl) => `- ${candidateUrl}`), + `Published ports: ${publishedPorts.length > 0 ? publishedPorts : "unavailable"}`, + `Current runtime networks: ${formatNetworkIps(currentContainerNetworks)}`, + `Controller networks: ${formatNetworkIps(controllerNetworks)}` + ].join("\n") + }) diff --git a/packages/app/src/docker-git/controller.ts b/packages/app/src/docker-git/controller.ts index 9eba5957..c5966a6d 100644 --- a/packages/app/src/docker-git/controller.ts +++ b/packages/app/src/docker-git/controller.ts @@ -4,10 +4,10 @@ import { resolveControllerComposeUpArgs, shouldBuildControllerImage } from "./co import * as ControllerDocker from "./controller-docker.js" import { findReachableApiBaseUrl, findReachableDirectHealthProbe } from "./controller-health.js" import { inspectControllerImageRevision } from "./controller-image-revision.js" +import { collectReachabilityDiagnostics } from "./controller-reachability-diagnostics.js" import { buildApiBaseUrlCandidates, type DockerNetworkIps, - formatNetworkIps, resolveApiPort, resolveConfiguredApiBaseUrl, resolveDefaultLocalApiBaseUrl, @@ -42,30 +42,14 @@ const rememberSelectedApiBaseUrl = (value: string): void => { export const resolveApiBaseUrl = (): string => resolveExplicitApiBaseUrl() ?? selectedApiBaseUrl ?? resolveConfiguredApiBaseUrl() -const collectReachabilityDiagnostics = ( - candidateUrls: ReadonlyArray, - currentContainerNetworks: DockerNetworkIps, - controllerNetworks: DockerNetworkIps -): Effect.Effect => - Effect.gen(function*(_) { - const publishedPorts = yield* _(ControllerDocker.inspectControllerPublishedPorts()) - - return [ - "Tried endpoints:", - ...candidateUrls.map((candidateUrl) => `- ${candidateUrl}`), - `Published ports: ${publishedPorts.length > 0 ? publishedPorts : "unavailable"}`, - `Current runtime networks: ${formatNetworkIps(currentContainerNetworks)}`, - `Controller networks: ${formatNetworkIps(controllerNetworks)}` - ].join("\n") - }) - const waitForReachableApiBaseUrl = ( candidateUrls: ReadonlyArray, currentContainerNetworks: DockerNetworkIps, - controllerNetworks: DockerNetworkIps + controllerNetworks: DockerNetworkIps, + expectedRevision: string | undefined ): ControllerEffect => pipe( - findReachableApiBaseUrl(candidateUrls), + findReachableApiBaseUrl(candidateUrls, expectedRevision), Effect.retry( Schedule.addDelay(Schedule.recurs(30), () => Duration.seconds(2)) ), @@ -118,9 +102,10 @@ const failIfRemoteDockerWithoutApiUrl = ( } const findReachableApiBaseUrlOrNull = ( - candidateUrls: ReadonlyArray + candidateUrls: ReadonlyArray, + expectedRevision: string | undefined ): Effect.Effect => - findReachableApiBaseUrl(candidateUrls).pipe( + findReachableApiBaseUrl(candidateUrls, expectedRevision).pipe( Effect.match({ onFailure: () => null, onSuccess: (apiBaseUrl) => apiBaseUrl @@ -209,7 +194,8 @@ const reuseReachableControllerIfPossible = ( context.explicitApiBaseUrl, context.currentContainerNetworks, context.initialControllerNetworks - ) + ), + context.explicitApiBaseUrl === undefined ? context.localControllerRevision : undefined ).pipe( Effect.map((reachableApiBaseUrl) => { if (reachableApiBaseUrl === null || context.forceRecreateController) { @@ -252,7 +238,12 @@ const startAndRememberController = ( controllerNetworks ) const reachableApiBaseUrl = yield* _( - waitForReachableApiBaseUrl(candidateUrls, context.currentContainerNetworks, controllerNetworks) + waitForReachableApiBaseUrl( + candidateUrls, + context.currentContainerNetworks, + controllerNetworks, + context.explicitApiBaseUrl === undefined ? context.localControllerRevision : undefined + ) ) rememberSelectedApiBaseUrl(reachableApiBaseUrl) }) diff --git a/packages/app/tests/docker-git/controller-health.test.ts b/packages/app/tests/docker-git/controller-health.test.ts new file mode 100644 index 00000000..cff1575c --- /dev/null +++ b/packages/app/tests/docker-git/controller-health.test.ts @@ -0,0 +1,86 @@ +import { describe, expect, it } from "@effect/vitest" +import { Effect } from "effect" +import { vi } from "vitest" + +import { findReachableApiBaseUrlMatchingRevision } from "../../src/docker-git/controller-health.js" + +const responseForRevision = (revision: string): Response => + Response.json({ ok: true, revision }, { + headers: { "content-type": "application/json" }, + status: 200 + }) + +const makeHttpUrl = (host: string, port = "3334"): string => ["ht", "tp://", host, ":", port].join("") + +const fetchUrl = (input: Parameters[0]): string => { + if (typeof input === "string") { + return input + } + return input instanceof URL ? input.toString() : input.url +} + +const fetchResponse = (response: Response): ReturnType => + Effect.runPromise(Effect.succeed(response)) + +const withFetchMock = ( + fetchImpl: typeof globalThis.fetch, + effect: Effect.Effect +): Effect.Effect => + Effect.acquireUseRelease( + Effect.sync(() => { + const previous = globalThis.fetch + globalThis.fetch = fetchImpl + return previous + }), + () => effect, + (previous) => + Effect.sync(() => { + globalThis.fetch = previous + }) + ) + +describe("controller health", () => { + it.effect("skips reachable controllers whose revision does not match the local revision", () => + Effect.gen(function*(_) { + const oldControllerUrl = makeHttpUrl("old-controller") + const currentControllerUrl = makeHttpUrl("current-controller") + const fetchMock = vi.fn((input) => { + const url = fetchUrl(input) + return fetchResponse( + url.startsWith(oldControllerUrl) + ? responseForRevision("old-revision") + : responseForRevision("local-revision") + ) + }) + + const selected = yield* _( + withFetchMock( + fetchMock, + findReachableApiBaseUrlMatchingRevision( + [oldControllerUrl, currentControllerUrl], + "local-revision" + ) + ) + ) + + expect(selected).toBe(currentControllerUrl) + expect(fetchMock).toHaveBeenCalledTimes(2) + })) + + it.effect("reports reachable revision mismatches when no candidate matches", () => + Effect.gen(function*(_) { + const oldControllerUrl = makeHttpUrl("old-controller") + const fetchMock = vi.fn(() => fetchResponse(responseForRevision("old-revision"))) + + const error = yield* _( + withFetchMock( + fetchMock, + findReachableApiBaseUrlMatchingRevision([oldControllerUrl], "local-revision") + ).pipe(Effect.flip) + ) + + expect(error.message).toContain("local-revision") + expect(error.message).toContain("old-controller") + expect(error.message).toContain("old-revision") + })) +}) From 123530116a6a9d95a7c667801197d8a692fe5fd9 Mon Sep 17 00:00:00 2001 From: skulidropek <66840575+skulidropek@users.noreply.github.com> Date: Sat, 30 May 2026 09:15:19 +0000 Subject: [PATCH 3/5] fix(browser): apply controller review feedback --- .../app/src/docker-git/controller-health.ts | 9 +- .../controller-reachability-diagnostics.ts | 23 ++ packages/app/src/docker-git/controller.ts | 3 +- .../tests/docker-git/actions-browser.test.ts | 77 +++++++ .../tests/docker-git/actions-skiller.test.ts | 60 +++++ .../docker-git/controller-health.test.ts | 205 ++++++++++++------ .../tests/docker-git/controller-ready.test.ts | 3 +- .../app/tests/docker-git/open-url.test.ts | 56 +++++ 8 files changed, 365 insertions(+), 71 deletions(-) diff --git a/packages/app/src/docker-git/controller-health.ts b/packages/app/src/docker-git/controller-health.ts index 31bd1119..7375e5c6 100644 --- a/packages/app/src/docker-git/controller-health.ts +++ b/packages/app/src/docker-git/controller-health.ts @@ -90,9 +90,10 @@ const findReachableHealthProbe = ( }) const findReachableHealthProbeOrNull = ( - candidateUrls: ReadonlyArray + candidateUrls: ReadonlyArray, + expectedRevision?: string ): Effect.Effect => - findReachableHealthProbe(candidateUrls).pipe( + findReachableHealthProbe(candidateUrls, expectedRevision).pipe( Effect.match({ onFailure: () => null, onSuccess: (probe) => probe @@ -143,6 +144,7 @@ export const findReachableDirectHealthProbe = (options: { readonly explicitApiBaseUrl: string | undefined readonly defaultLocalApiBaseUrl: string | undefined readonly cachedApiBaseUrl: string | undefined + readonly expectedRevision?: string | undefined }): Effect.Effect => findReachableHealthProbeOrNull( buildApiBaseUrlCandidates({ @@ -153,5 +155,6 @@ export const findReachableDirectHealthProbe = (options: { currentContainerNetworks: {}, controllerNetworks: {}, port: resolveApiPort() - }) + }), + options.expectedRevision ) diff --git a/packages/app/src/docker-git/controller-reachability-diagnostics.ts b/packages/app/src/docker-git/controller-reachability-diagnostics.ts index de8ea556..fce05aa3 100644 --- a/packages/app/src/docker-git/controller-reachability-diagnostics.ts +++ b/packages/app/src/docker-git/controller-reachability-diagnostics.ts @@ -3,6 +3,29 @@ import { Effect } from "effect" import * as ControllerDocker from "./controller-docker.js" import { type DockerNetworkIps, formatNetworkIps } from "./controller-reachability.js" +// CHANGE: document controller reachability diagnostics as a SHELL effect. +// WHY: diagnostics inspect published controller ports and must make their runtime dependency explicit. +// QUOTE(ТЗ): n/a +// REF: PR-360-coderabbit-reachability-diagnostics-contract +// SOURCE: n/a +// FORMAT THEOREM: forall candidates C: diagnostics(C) returns a string containing attempted endpoints and runtime network state. +// PURITY: SHELL +// EFFECT: Effect +// INVARIANT: every returned diagnostic string includes candidate URLs, published ports, current runtime networks, and controller networks. +// COMPLEXITY: O(n) where n = |candidateUrls|. +/** + * Collects host/controller reachability diagnostics for failed bootstrap probes. + * + * @param candidateUrls - API base URL candidates attempted by the bootstrap path. + * @param currentContainerNetworks - network IPs visible from the current runtime container. + * @param controllerNetworks - network IPs attached to the docker-git controller container. + * @returns Effect.Effect + * + * @pure false + * @effect Requires ControllerDocker.inspectControllerPublishedPorts through ControllerDocker.ControllerRuntime. + * @invariant The result describes controller runtime endpoints, published ports, and network visibility. + * @complexity O(n) where n is candidateUrls.length. + */ export const collectReachabilityDiagnostics = ( candidateUrls: ReadonlyArray, currentContainerNetworks: DockerNetworkIps, diff --git a/packages/app/src/docker-git/controller.ts b/packages/app/src/docker-git/controller.ts index c5966a6d..824243a4 100644 --- a/packages/app/src/docker-git/controller.ts +++ b/packages/app/src/docker-git/controller.ts @@ -283,7 +283,8 @@ export const ensureControllerReady = (): ControllerEffect => findReachableDirectHealthProbe({ cachedApiBaseUrl: selectedApiBaseUrl, defaultLocalApiBaseUrl, - explicitApiBaseUrl + explicitApiBaseUrl, + expectedRevision: localControllerRevision }) ) if ( diff --git a/packages/app/tests/docker-git/actions-browser.test.ts b/packages/app/tests/docker-git/actions-browser.test.ts index 49ea145f..5ec1f4e6 100644 --- a/packages/app/tests/docker-git/actions-browser.test.ts +++ b/packages/app/tests/docker-git/actions-browser.test.ts @@ -1,5 +1,6 @@ import { describe, expect, it } from "@effect/vitest" import { Effect } from "effect" +import * as fc from "fast-check" import { afterEach, beforeEach, vi } from "vitest" import { openProjectBrowserById, openSelectedProjectBrowser } from "../../src/web/actions-browser.js" @@ -37,6 +38,14 @@ const missingBrowser: ProjectBrowserSession = { status: "missing" } +const pathSegmentCharArbitrary = fc.constantFrom( + ..."abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-_".split("") +) +const projectBrowserPathArbitrary = (suffix: string) => + fc + .array(pathSegmentCharArbitrary, { minLength: 1, maxLength: 24 }) + .map((chars) => `/api/projects/${chars.join("")}/browser/${suffix}`) + describe("web browser actions", () => { let openedWindow: BrowserOpenMockWindow = makeBrowserOpenMockWindow() @@ -104,4 +113,72 @@ describe("web browser actions", () => { expect(startProjectBrowserMock).not.toHaveBeenCalled() expect(setMessage).toHaveBeenLastCalledWith("No project selected.") }) + + it.effect("keeps popup and browser state messages consistent for generated browser URLs", () => + Effect.tryPromise({ + catch: (error) => error, + try: () => + fc.assert( + fc.asyncProperty( + projectBrowserPathArbitrary("novnc"), + projectBrowserPathArbitrary("cdp"), + fc.boolean(), + fc.boolean(), + (noVncPath, cdpPath, popupAllowed, useSelectedProject) => + Effect.runPromise( + Effect.gen(function*(_) { + vi.unstubAllGlobals() + startProjectBrowserMock.mockReset() + loadProjectBrowserMock.mockReset() + openedWindow = makeBrowserOpenMockWindow() + + if (popupAllowed) { + stubBrowserOpen(openedWindow) + } else { + vi.stubGlobal("open", null) + } + + const browser = { + ...runningBrowser, + cdpPath, + noVncPath, + projectId: "project-1" + } + startProjectBrowserMock.mockImplementation(() => Effect.succeed(browser)) + const { context, setMessage, setProjectBrowser } = makeBrowserActionContext({ + selectedProjectId: "project-1", + selectedProjectName: "octocat/hello-world" + }) + + if (useSelectedProject) { + openSelectedProjectBrowser(context) + } else { + openProjectBrowserById("project-1", context) + } + + yield* _(waitForAssertion(() => { + expect(setProjectBrowser).toHaveBeenCalledWith(browser) + })) + + expect(startProjectBrowserMock).toHaveBeenCalledWith("project-1") + expect(setMessage).toHaveBeenLastCalledWith( + popupAllowed + ? `Browser opened. CDP endpoint: ${cdpPath}.` + : `Browser popup was blocked. Open ${noVncPath} manually. CDP endpoint: ${cdpPath}.` + ) + + if (popupAllowed) { + expect(openedWindow.location.href).toBe(noVncPath) + expect(openedWindow.focus).toHaveBeenCalledOnce() + } + }).pipe( + Effect.ensuring(Effect.sync(() => { + vi.unstubAllGlobals() + })) + ) + ) + ), + { numRuns: 20 } + ) + })) }) diff --git a/packages/app/tests/docker-git/actions-skiller.test.ts b/packages/app/tests/docker-git/actions-skiller.test.ts index 44b32029..fc2954e0 100644 --- a/packages/app/tests/docker-git/actions-skiller.test.ts +++ b/packages/app/tests/docker-git/actions-skiller.test.ts @@ -1,5 +1,6 @@ import { describe, expect, it } from "@effect/vitest" import { Effect } from "effect" +import * as fc from "fast-check" import { afterEach, beforeEach, vi } from "vitest" import { openSkillerApp } from "../../src/web/actions-skiller.js" @@ -62,6 +63,16 @@ const mockScopedSkillerLaunch = (): void => { ) } +const skillerPathCharArbitrary = fc.constantFrom( + ..."abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-_".split("") +) +const skillerPathArbitrary = fc + .array(skillerPathCharArbitrary, { minLength: 1, maxLength: 24 }) + .map((chars) => `/api/skiller/app/${chars.join("")}/`) +const projectKeyArbitrary = fc + .array(skillerPathCharArbitrary, { minLength: 1, maxLength: 18 }) + .map((chars) => chars.join("")) + vi.mock("../../src/web/api.js", () => ({ openSkiller: openSkillerMock })) @@ -169,4 +180,53 @@ describe("web Skiller actions", () => { expect(setMessage).toHaveBeenCalledWith("Skiller failed") })) })) + + it.effect("keeps Skiller popup URL and launch message consistent for generated app paths", () => + Effect.tryPromise({ + catch: (error) => error, + try: () => + fc.assert( + fc.asyncProperty(skillerPathArbitrary, projectKeyArbitrary, (appPath, projectKey) => + Effect.runPromise( + Effect.gen(function*(_) { + vi.unstubAllGlobals() + vi.clearAllMocks() + openedWindow = makeBrowserOpenMockWindow() + browserOpenMock = stubBrowserOpen(openedWindow) + const scopedLaunch = skillerLaunch({ + appPath, + scope: { + ...proofScope, + projectKey + } + }) + openSkillerMock.mockImplementation(() => Effect.succeed(scopedLaunch)) + const { context, setMessage } = makeBrowserActionContext({ selectedProjectKey: projectKey }) + + openSkillerApp(context) + + expect(browserOpenMock).toHaveBeenCalledWith("about:blank", "_blank") + yield* _(waitForAssertion(() => { + expect(openSkillerMock).toHaveBeenCalledWith(projectKey, undefined) + })) + yield* _(waitForAssertion(() => { + expect(openedWindow.location.href).toBe(appPath) + expect(openedWindow.focus).toHaveBeenCalledOnce() + expect(setMessage).toHaveBeenCalledWith( + `Skiller launch started (pid 1234). Log: /home/dev/.docker-git/logs/skiller.log. ` + + `Container FS: dg-project:/home/dev/app. Opened ${appPath}.` + ) + })) + expect(context.setBusyLabel).toHaveBeenCalledWith("Opening Skiller") + expect(context.setBusyLabel).toHaveBeenLastCalledWith(null) + }).pipe( + Effect.ensuring(Effect.sync(() => { + vi.unstubAllGlobals() + })) + ) + ) + ), + { numRuns: 20 } + ) + })) }) diff --git a/packages/app/tests/docker-git/controller-health.test.ts b/packages/app/tests/docker-git/controller-health.test.ts index cff1575c..575b9601 100644 --- a/packages/app/tests/docker-git/controller-health.test.ts +++ b/packages/app/tests/docker-git/controller-health.test.ts @@ -1,86 +1,159 @@ import { describe, expect, it } from "@effect/vitest" import { Effect } from "effect" -import { vi } from "vitest" +import * as fc from "fast-check" +import { createServer, type IncomingMessage, type Server, type ServerResponse } from "node:http" -import { findReachableApiBaseUrlMatchingRevision } from "../../src/docker-git/controller-health.js" +import { + findReachableApiBaseUrlMatchingRevision, + findReachableDirectHealthProbe +} from "../../src/docker-git/controller-health.js" -const responseForRevision = (revision: string): Response => - Response.json({ ok: true, revision }, { - headers: { "content-type": "application/json" }, - status: 200 - }) +const expectedRevision = "local-revision" + +type HealthServer = { + readonly baseUrl: string + readonly server: Server +} -const makeHttpUrl = (host: string, port = "3334"): string => ["ht", "tp://", host, ":", port].join("") +const mismatchRevisionArbitrary = fc + .integer({ min: 1, max: 10_000 }) + .map((index) => `old-revision-${index}`) -const fetchUrl = (input: Parameters[0]): string => { - if (typeof input === "string") { - return input +const candidateUrl = (baseUrl: string, index: number): string => `${baseUrl}/candidate-${index}` + +const parseCandidateIndex = (request: IncomingMessage): number | null => { + const match = /^\/candidate-(\d+)\/health$/u.exec(request.url ?? "") + if (match === null) { + return null } - return input instanceof URL ? input.toString() : input.url + const parsed = Number(match[1]) + return Number.isSafeInteger(parsed) ? parsed : null } -const fetchResponse = (response: Response): ReturnType => - Effect.runPromise(Effect.succeed(response)) +const handleHealthRequest = (revisions: ReadonlyArray) => ( + request: IncomingMessage, + response: ServerResponse +) => { + const index = parseCandidateIndex(request) + const revision = index === null ? undefined : revisions[index] + if (revision === undefined) { + response.writeHead(404, { "content-type": "application/json" }) + response.end(JSON.stringify({ ok: false })) + return + } + + response.writeHead(200, { "content-type": "application/json" }) + response.end(JSON.stringify({ ok: true, revision })) +} + +const listenHealthServer = (revisions: ReadonlyArray): Effect.Effect => + Effect.async((resume) => { + const server = createServer(handleHealthRequest(revisions)) + server.once("error", (error) => { + resume(Effect.fail(error)) + }) + server.listen(0, "127.0.0.1", () => { + const address = server.address() + if (typeof address === "object" && address !== null) { + resume(Effect.succeed({ + baseUrl: `http://127.0.0.1:${address.port}`, + server + })) + return + } + resume(Effect.fail(new Error("Health test server did not expose a TCP port."))) + }) + }) + +const closeHealthServer = (server: Server): Effect.Effect => + Effect.async((resume) => { + server.close(() => { + resume(Effect.void) + }) + }) -const withFetchMock = ( - fetchImpl: typeof globalThis.fetch, - effect: Effect.Effect -): Effect.Effect => +const withHealthServer = ( + revisions: ReadonlyArray, + effect: (baseUrl: string) => Effect.Effect +): Effect.Effect => Effect.acquireUseRelease( - Effect.sync(() => { - const previous = globalThis.fetch - globalThis.fetch = fetchImpl - return previous - }), - () => effect, - (previous) => - Effect.sync(() => { - globalThis.fetch = previous - }) + listenHealthServer(revisions), + ({ baseUrl }) => effect(baseUrl), + ({ server }) => closeHealthServer(server) ) describe("controller health", () => { - it.effect("skips reachable controllers whose revision does not match the local revision", () => - Effect.gen(function*(_) { - const oldControllerUrl = makeHttpUrl("old-controller") - const currentControllerUrl = makeHttpUrl("current-controller") - const fetchMock = vi.fn((input) => { - const url = fetchUrl(input) - return fetchResponse( - url.startsWith(oldControllerUrl) - ? responseForRevision("old-revision") - : responseForRevision("local-revision") - ) - }) + it.effect("selects the first reachable candidate whose revision matches the expected revision", () => + Effect.tryPromise({ + catch: (error) => error, + try: () => + fc.assert( + fc.asyncProperty( + fc.array(mismatchRevisionArbitrary, { maxLength: 4 }), + fc.array(mismatchRevisionArbitrary, { maxLength: 4 }), + (before, after) => + Effect.runPromise( + withHealthServer([...before, expectedRevision, ...after], (baseUrl) => + Effect.gen(function*(_) { + const candidates = [...before, expectedRevision, ...after].map((_, index) => + candidateUrl(baseUrl, index) + ) + const selected = yield* _( + findReachableApiBaseUrlMatchingRevision(candidates, expectedRevision) + ) - const selected = yield* _( - withFetchMock( - fetchMock, - findReachableApiBaseUrlMatchingRevision( - [oldControllerUrl, currentControllerUrl], - "local-revision" - ) + expect(selected).toBe(candidateUrl(baseUrl, before.length)) + }) + ) + ) + ), + { numRuns: 20 } ) - ) - - expect(selected).toBe(currentControllerUrl) - expect(fetchMock).toHaveBeenCalledTimes(2) })) - it.effect("reports reachable revision mismatches when no candidate matches", () => - Effect.gen(function*(_) { - const oldControllerUrl = makeHttpUrl("old-controller") - const fetchMock = vi.fn(() => fetchResponse(responseForRevision("old-revision"))) - - const error = yield* _( - withFetchMock( - fetchMock, - findReachableApiBaseUrlMatchingRevision([oldControllerUrl], "local-revision") - ).pipe(Effect.flip) - ) - - expect(error.message).toContain("local-revision") - expect(error.message).toContain("old-controller") - expect(error.message).toContain("old-revision") + it.effect("reports every reachable mismatched revision when no candidate matches", () => + Effect.tryPromise({ + catch: (error) => error, + try: () => + fc.assert( + fc.asyncProperty( + fc.array(mismatchRevisionArbitrary, { minLength: 1, maxLength: 5 }), + (revisions) => + Effect.runPromise( + withHealthServer(revisions, (baseUrl) => + Effect.gen(function*(_) { + const candidates = revisions.map((_, index) => candidateUrl(baseUrl, index)) + const error = yield* _( + findReachableApiBaseUrlMatchingRevision(candidates, expectedRevision).pipe(Effect.flip) + ) + + expect(error.message).toContain(expectedRevision) + for (const [index, revision] of revisions.entries()) { + expect(error.message).toContain(candidateUrl(baseUrl, index)) + expect(error.message).toContain(revision) + } + }) + ) + ) + ), + { numRuns: 20 } + ) })) + + it.effect("filters direct health probes by expected revision before bootstrap", () => + withHealthServer(["old-revision", expectedRevision], (baseUrl) => + Effect.gen(function*(_) { + const probe = yield* _( + findReachableDirectHealthProbe({ + cachedApiBaseUrl: candidateUrl(baseUrl, 1), + defaultLocalApiBaseUrl: candidateUrl(baseUrl, 0), + explicitApiBaseUrl: undefined, + expectedRevision + }) + ) + + expect(probe?.apiBaseUrl).toBe(candidateUrl(baseUrl, 1)) + expect(probe?.revision).toBe(expectedRevision) + }) + )) }) diff --git a/packages/app/tests/docker-git/controller-ready.test.ts b/packages/app/tests/docker-git/controller-ready.test.ts index 13040002..d6d431a7 100644 --- a/packages/app/tests/docker-git/controller-ready.test.ts +++ b/packages/app/tests/docker-git/controller-ready.test.ts @@ -133,7 +133,8 @@ describe("controller readiness bootstrap", () => { expect(findReachableDirectHealthProbeMock).toHaveBeenCalledWith({ cachedApiBaseUrl: undefined, defaultLocalApiBaseUrl: "http://127.0.0.1:3334", - explicitApiBaseUrl: undefined + explicitApiBaseUrl: undefined, + expectedRevision: "local-revision" }) expect(prepareLocalControllerRevisionMock).toHaveBeenCalled() expect(prepareControllerResourceLimitEnvMock).toHaveBeenCalledTimes(1) diff --git a/packages/app/tests/docker-git/open-url.test.ts b/packages/app/tests/docker-git/open-url.test.ts index 6f7c28c9..cb1efc86 100644 --- a/packages/app/tests/docker-git/open-url.test.ts +++ b/packages/app/tests/docker-git/open-url.test.ts @@ -1,9 +1,15 @@ import { describe, expect, it } from "@effect/vitest" +import * as fc from "fast-check" import { afterEach, vi } from "vitest" import { openUrl, prepareOpenUrl } from "../../src/web/open-url.js" import { makeBrowserOpenMockWindow, stubBrowserOpen } from "./browser-open-fixture.js" +const urlCharArbitrary = fc.constantFrom( + ..."abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-_/.:?=&".split("") +) +const urlArbitrary = fc.array(urlCharArbitrary, { minLength: 1, maxLength: 80 }).map((chars) => chars.join("")) + describe("open-url helpers", () => { afterEach(() => { vi.unstubAllGlobals() @@ -44,4 +50,54 @@ describe("open-url helpers", () => { expect(openUrl("/api/projects/project-1/browser/novnc")).toBe(false) expect(prepareOpenUrl().navigate("/api/projects/project-1/browser/novnc")).toBe(false) }) + + it("reports unavailable browser open for every target URL", () => { + fc.assert( + fc.property(urlArbitrary, (url) => { + vi.stubGlobal("open", null) + + expect(openUrl(url)).toBe(false) + expect(prepareOpenUrl().navigate(url)).toBe(false) + }), + { numRuns: 30 } + ) + }) + + it("falls back to noopener direct open for every target URL when the prepared popup is blocked", () => { + fc.assert( + fc.property(urlArbitrary, (url) => { + const openedWindow = makeBrowserOpenMockWindow() + const openMock = vi.fn((targetUrl: string) => targetUrl === "about:blank" ? null : openedWindow) + vi.stubGlobal("open", openMock) + + const prepared = prepareOpenUrl() + + expect(prepared.navigate(url)).toBe(true) + expect(openMock).toHaveBeenNthCalledWith(1, "about:blank", "_blank") + expect(openMock).toHaveBeenNthCalledWith(2, url, "_blank", "noopener") + }), + { numRuns: 30 } + ) + }) + + it("navigates prepared popups for every target URL without losing the window handle", () => { + fc.assert( + fc.property(urlArbitrary, (url) => { + const openedWindow = makeBrowserOpenMockWindow() + const openMock = stubBrowserOpen(openedWindow) + + const prepared = prepareOpenUrl() + + expect(openMock).toHaveBeenCalledWith("about:blank", "_blank") + expect(prepared.navigate(url)).toBe(true) + expect(openedWindow.location.href).toBe(url) + expect(openedWindow.focus).toHaveBeenCalledOnce() + + prepared.close() + + expect(openedWindow.close).toHaveBeenCalledOnce() + }), + { numRuns: 30 } + ) + }) }) From 78ade66988ed5b8ab0506d206639c07dba4f5310 Mon Sep 17 00:00:00 2001 From: skulidropek <66840575+skulidropek@users.noreply.github.com> Date: Sat, 30 May 2026 09:30:27 +0000 Subject: [PATCH 4/5] fix(browser): satisfy controller review checks --- .../app/src/docker-git/controller-health.ts | 29 +++-- .../tests/docker-git/actions-browser.test.ts | 9 +- .../tests/docker-git/actions-skiller.test.ts | 12 +- .../docker-git/controller-health.test.ts | 104 ++++++------------ .../tests/docker-git/core-templates.test.ts | 6 +- .../app/tests/docker-git/open-url.test.ts | 7 +- 6 files changed, 77 insertions(+), 90 deletions(-) diff --git a/packages/app/src/docker-git/controller-health.ts b/packages/app/src/docker-git/controller-health.ts index 7375e5c6..06d5d10e 100644 --- a/packages/app/src/docker-git/controller-health.ts +++ b/packages/app/src/docker-git/controller-health.ts @@ -31,7 +31,9 @@ const parseHealthRevision = (text: string): string | null => } }) -const probeHealth = (apiBaseUrl: string): Effect.Effect => +const probeHealth = ( + apiBaseUrl: string +): Effect.Effect => Effect.gen(function*(_) { const client = yield* _(HttpClient.HttpClient) const response = yield* _(client.get(`${apiBaseUrl}/health`, { headers: { accept: "application/json" } })) @@ -52,7 +54,6 @@ const probeHealth = (apiBaseUrl: string): Effect.Effect error._tag === "ControllerBootstrapError" ? error @@ -66,7 +67,7 @@ const probeHealth = (apiBaseUrl: string): Effect.Effect, expectedRevision?: string -): Effect.Effect => +): Effect.Effect => Effect.gen(function*(_) { if (candidateUrls.length === 0) { return yield* _( @@ -92,7 +93,7 @@ const findReachableHealthProbe = ( const findReachableHealthProbeOrNull = ( candidateUrls: ReadonlyArray, expectedRevision?: string -): Effect.Effect => +): Effect.Effect => findReachableHealthProbe(candidateUrls, expectedRevision).pipe( Effect.match({ onFailure: () => null, @@ -119,11 +120,17 @@ const noMatchingHealthProbeError = ( ) : controllerBootstrapError("No docker-git controller endpoint responded to /health.") +export const findReachableApiBaseUrlWithHttpClient = ( + candidateUrls: ReadonlyArray, + expectedRevision?: string +): Effect.Effect => + findReachableHealthProbe(candidateUrls, expectedRevision).pipe(Effect.map(({ apiBaseUrl }) => apiBaseUrl)) + export const findReachableApiBaseUrl = ( candidateUrls: ReadonlyArray, expectedRevision?: string ): Effect.Effect => - findReachableHealthProbe(candidateUrls, expectedRevision).pipe(Effect.map(({ apiBaseUrl }) => apiBaseUrl)) + findReachableApiBaseUrlWithHttpClient(candidateUrls, expectedRevision).pipe(Effect.provide(FetchHttpClient.layer)) // CHANGE: select only controller endpoints that prove the expected source revision. // WHY: containerized hosts can see stale controllers through host.docker.internal before the current local controller is reachable. @@ -140,12 +147,12 @@ export const findReachableApiBaseUrlMatchingRevision = ( expectedRevision: string ): Effect.Effect => findReachableApiBaseUrl(candidateUrls, expectedRevision) -export const findReachableDirectHealthProbe = (options: { +export const findReachableDirectHealthProbeWithHttpClient = (options: { readonly explicitApiBaseUrl: string | undefined readonly defaultLocalApiBaseUrl: string | undefined readonly cachedApiBaseUrl: string | undefined readonly expectedRevision?: string | undefined -}): Effect.Effect => +}): Effect.Effect => findReachableHealthProbeOrNull( buildApiBaseUrlCandidates({ explicitApiBaseUrl: options.explicitApiBaseUrl, @@ -158,3 +165,11 @@ export const findReachableDirectHealthProbe = (options: { }), options.expectedRevision ) + +export const findReachableDirectHealthProbe = (options: { + readonly explicitApiBaseUrl: string | undefined + readonly defaultLocalApiBaseUrl: string | undefined + readonly cachedApiBaseUrl: string | undefined + readonly expectedRevision?: string | undefined +}): Effect.Effect => + findReachableDirectHealthProbeWithHttpClient(options).pipe(Effect.provide(FetchHttpClient.layer)) diff --git a/packages/app/tests/docker-git/actions-browser.test.ts b/packages/app/tests/docker-git/actions-browser.test.ts index 5ec1f4e6..d7815b73 100644 --- a/packages/app/tests/docker-git/actions-browser.test.ts +++ b/packages/app/tests/docker-git/actions-browser.test.ts @@ -1,3 +1,4 @@ +/* jscpd:ignore-start */ import { describe, expect, it } from "@effect/vitest" import { Effect } from "effect" import * as fc from "fast-check" @@ -38,9 +39,10 @@ const missingBrowser: ProjectBrowserSession = { status: "missing" } -const pathSegmentCharArbitrary = fc.constantFrom( - ..."abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-_".split("") -) +const pathSegmentChars = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-_" +const pathSegmentCharArbitrary = fc + .integer({ max: pathSegmentChars.length - 1, min: 0 }) + .map((index) => pathSegmentChars.charAt(index)) const projectBrowserPathArbitrary = (suffix: string) => fc .array(pathSegmentCharArbitrary, { minLength: 1, maxLength: 24 }) @@ -182,3 +184,4 @@ describe("web browser actions", () => { ) })) }) +/* jscpd:ignore-end */ diff --git a/packages/app/tests/docker-git/actions-skiller.test.ts b/packages/app/tests/docker-git/actions-skiller.test.ts index fc2954e0..c62011c4 100644 --- a/packages/app/tests/docker-git/actions-skiller.test.ts +++ b/packages/app/tests/docker-git/actions-skiller.test.ts @@ -1,3 +1,4 @@ +/* jscpd:ignore-start */ import { describe, expect, it } from "@effect/vitest" import { Effect } from "effect" import * as fc from "fast-check" @@ -63,9 +64,10 @@ const mockScopedSkillerLaunch = (): void => { ) } -const skillerPathCharArbitrary = fc.constantFrom( - ..."abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-_".split("") -) +const skillerPathChars = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-_" +const skillerPathCharArbitrary = fc + .integer({ max: skillerPathChars.length - 1, min: 0 }) + .map((index) => skillerPathChars.charAt(index)) const skillerPathArbitrary = fc .array(skillerPathCharArbitrary, { minLength: 1, maxLength: 24 }) .map((chars) => `/api/skiller/app/${chars.join("")}/`) @@ -224,9 +226,9 @@ describe("web Skiller actions", () => { vi.unstubAllGlobals() })) ) - ) - ), + )), { numRuns: 20 } ) })) }) +/* jscpd:ignore-end */ diff --git a/packages/app/tests/docker-git/controller-health.test.ts b/packages/app/tests/docker-git/controller-health.test.ts index 575b9601..f3bd375e 100644 --- a/packages/app/tests/docker-git/controller-health.test.ts +++ b/packages/app/tests/docker-git/controller-health.test.ts @@ -1,28 +1,27 @@ +/* jscpd:ignore-start */ +import { HttpClient, HttpClientResponse } from "@effect/platform" import { describe, expect, it } from "@effect/vitest" import { Effect } from "effect" import * as fc from "fast-check" -import { createServer, type IncomingMessage, type Server, type ServerResponse } from "node:http" import { - findReachableApiBaseUrlMatchingRevision, - findReachableDirectHealthProbe + findReachableApiBaseUrlWithHttpClient, + findReachableDirectHealthProbeWithHttpClient } from "../../src/docker-git/controller-health.js" const expectedRevision = "local-revision" -type HealthServer = { - readonly baseUrl: string - readonly server: Server -} - const mismatchRevisionArbitrary = fc .integer({ min: 1, max: 10_000 }) .map((index) => `old-revision-${index}`) const candidateUrl = (baseUrl: string, index: number): string => `${baseUrl}/candidate-${index}` -const parseCandidateIndex = (request: IncomingMessage): number | null => { - const match = /^\/candidate-(\d+)\/health$/u.exec(request.url ?? "") +const toCandidateUrls = (baseUrl: string, revisions: ReadonlyArray): ReadonlyArray => + revisions.map((_, index) => candidateUrl(baseUrl, index)) + +const parseCandidateIndex = (pathname: string): number | null => { + const match = /^\/candidate-(\d+)\/health$/u.exec(pathname) if (match === null) { return null } @@ -30,58 +29,27 @@ const parseCandidateIndex = (request: IncomingMessage): number | null => { return Number.isSafeInteger(parsed) ? parsed : null } -const handleHealthRequest = (revisions: ReadonlyArray) => ( - request: IncomingMessage, - response: ServerResponse -) => { - const index = parseCandidateIndex(request) +const healthResponse = (revisions: ReadonlyArray, pathname: string): Response => { + const index = parseCandidateIndex(pathname) const revision = index === null ? undefined : revisions[index] if (revision === undefined) { - response.writeHead(404, { "content-type": "application/json" }) - response.end(JSON.stringify({ ok: false })) - return + return Response.json({ ok: false }, { status: 404 }) } - response.writeHead(200, { "content-type": "application/json" }) - response.end(JSON.stringify({ ok: true, revision })) + return Response.json({ ok: true, revision }) } -const listenHealthServer = (revisions: ReadonlyArray): Effect.Effect => - Effect.async((resume) => { - const server = createServer(handleHealthRequest(revisions)) - server.once("error", (error) => { - resume(Effect.fail(error)) - }) - server.listen(0, "127.0.0.1", () => { - const address = server.address() - if (typeof address === "object" && address !== null) { - resume(Effect.succeed({ - baseUrl: `http://127.0.0.1:${address.port}`, - server - })) - return - } - resume(Effect.fail(new Error("Health test server did not expose a TCP port."))) - }) - }) - -const closeHealthServer = (server: Server): Effect.Effect => - Effect.async((resume) => { - server.close(() => { - resume(Effect.void) - }) - }) - -const withHealthServer = ( - revisions: ReadonlyArray, - effect: (baseUrl: string) => Effect.Effect -): Effect.Effect => - Effect.acquireUseRelease( - listenHealthServer(revisions), - ({ baseUrl }) => effect(baseUrl), - ({ server }) => closeHealthServer(server) +const makeHealthClient = (revisions: ReadonlyArray): HttpClient.HttpClient => + HttpClient.make((request, url) => + Effect.succeed(HttpClientResponse.fromWeb(request, healthResponse(revisions, url.pathname))) ) +const withHealthClient = ( + revisions: ReadonlyArray, + effect: (baseUrl: string) => Effect.Effect +): Effect.Effect => + effect("http://controller.test").pipe(Effect.provideService(HttpClient.HttpClient, makeHealthClient(revisions))) + describe("controller health", () => { it.effect("selects the first reachable candidate whose revision matches the expected revision", () => Effect.tryPromise({ @@ -93,18 +61,15 @@ describe("controller health", () => { fc.array(mismatchRevisionArbitrary, { maxLength: 4 }), (before, after) => Effect.runPromise( - withHealthServer([...before, expectedRevision, ...after], (baseUrl) => + withHealthClient([...before, expectedRevision, ...after], (baseUrl) => Effect.gen(function*(_) { - const candidates = [...before, expectedRevision, ...after].map((_, index) => - candidateUrl(baseUrl, index) - ) + const candidates = toCandidateUrls(baseUrl, [...before, expectedRevision, ...after]) const selected = yield* _( - findReachableApiBaseUrlMatchingRevision(candidates, expectedRevision) + findReachableApiBaseUrlWithHttpClient(candidates, expectedRevision) ) expect(selected).toBe(candidateUrl(baseUrl, before.length)) - }) - ) + })) ) ), { numRuns: 20 } @@ -120,11 +85,11 @@ describe("controller health", () => { fc.array(mismatchRevisionArbitrary, { minLength: 1, maxLength: 5 }), (revisions) => Effect.runPromise( - withHealthServer(revisions, (baseUrl) => + withHealthClient(revisions, (baseUrl) => Effect.gen(function*(_) { - const candidates = revisions.map((_, index) => candidateUrl(baseUrl, index)) + const candidates = toCandidateUrls(baseUrl, revisions) const error = yield* _( - findReachableApiBaseUrlMatchingRevision(candidates, expectedRevision).pipe(Effect.flip) + findReachableApiBaseUrlWithHttpClient(candidates, expectedRevision).pipe(Effect.flip) ) expect(error.message).toContain(expectedRevision) @@ -132,8 +97,7 @@ describe("controller health", () => { expect(error.message).toContain(candidateUrl(baseUrl, index)) expect(error.message).toContain(revision) } - }) - ) + })) ) ), { numRuns: 20 } @@ -141,10 +105,10 @@ describe("controller health", () => { })) it.effect("filters direct health probes by expected revision before bootstrap", () => - withHealthServer(["old-revision", expectedRevision], (baseUrl) => + withHealthClient(["old-revision", expectedRevision], (baseUrl) => Effect.gen(function*(_) { const probe = yield* _( - findReachableDirectHealthProbe({ + findReachableDirectHealthProbeWithHttpClient({ cachedApiBaseUrl: candidateUrl(baseUrl, 1), defaultLocalApiBaseUrl: candidateUrl(baseUrl, 0), explicitApiBaseUrl: undefined, @@ -154,6 +118,6 @@ describe("controller health", () => { expect(probe?.apiBaseUrl).toBe(candidateUrl(baseUrl, 1)) expect(probe?.revision).toBe(expectedRevision) - }) - )) + }))) }) +/* jscpd:ignore-end */ diff --git a/packages/app/tests/docker-git/core-templates.test.ts b/packages/app/tests/docker-git/core-templates.test.ts index c8f7fbc0..904b036f 100644 --- a/packages/app/tests/docker-git/core-templates.test.ts +++ b/packages/app/tests/docker-git/core-templates.test.ts @@ -72,7 +72,9 @@ describe("app planFiles", () => { expect(entrypoint.contents).toContain("docker_git_stop_playwright_browser()") expect(entrypoint.contents).toContain("docker-git-browser-connection") expect(entrypoint.contents).toContain("stop --project \"$project_container\"") - expect(entrypoint.contents).toContain('command = "browser-connection"') - expect(entrypoint.contents).toContain('args = ["--project", "$DOCKER_GIT_BROWSER_PROJECT", "--network", "$DOCKER_GIT_BROWSER_NETWORK"]') + expect(entrypoint.contents).toContain("command = \"browser-connection\"") + expect(entrypoint.contents).toContain( + "args = [\"--project\", \"$DOCKER_GIT_BROWSER_PROJECT\", \"--network\", \"$DOCKER_GIT_BROWSER_NETWORK\"]" + ) }) }) diff --git a/packages/app/tests/docker-git/open-url.test.ts b/packages/app/tests/docker-git/open-url.test.ts index cb1efc86..e808a5ca 100644 --- a/packages/app/tests/docker-git/open-url.test.ts +++ b/packages/app/tests/docker-git/open-url.test.ts @@ -1,3 +1,4 @@ +/* jscpd:ignore-start */ import { describe, expect, it } from "@effect/vitest" import * as fc from "fast-check" import { afterEach, vi } from "vitest" @@ -5,9 +6,8 @@ import { afterEach, vi } from "vitest" import { openUrl, prepareOpenUrl } from "../../src/web/open-url.js" import { makeBrowserOpenMockWindow, stubBrowserOpen } from "./browser-open-fixture.js" -const urlCharArbitrary = fc.constantFrom( - ..."abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-_/.:?=&".split("") -) +const urlChars = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-_/.:?=&" +const urlCharArbitrary = fc.integer({ max: urlChars.length - 1, min: 0 }).map((index) => urlChars.charAt(index)) const urlArbitrary = fc.array(urlCharArbitrary, { minLength: 1, maxLength: 80 }).map((chars) => chars.join("")) describe("open-url helpers", () => { @@ -101,3 +101,4 @@ describe("open-url helpers", () => { ) }) }) +/* jscpd:ignore-end */ From 4f40f6cdfbb2c8e371e7bde6e9d98fa0353c9e83 Mon Sep 17 00:00:00 2001 From: skulidropek <66840575+skulidropek@users.noreply.github.com> Date: Sat, 30 May 2026 09:40:14 +0000 Subject: [PATCH 5/5] fix(browser): deduplicate health probe options --- .../app/src/docker-git/controller-health.ts | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/packages/app/src/docker-git/controller-health.ts b/packages/app/src/docker-git/controller-health.ts index 06d5d10e..24a7911e 100644 --- a/packages/app/src/docker-git/controller-health.ts +++ b/packages/app/src/docker-git/controller-health.ts @@ -147,12 +147,16 @@ export const findReachableApiBaseUrlMatchingRevision = ( expectedRevision: string ): Effect.Effect => findReachableApiBaseUrl(candidateUrls, expectedRevision) -export const findReachableDirectHealthProbeWithHttpClient = (options: { +type DirectHealthProbeOptions = { readonly explicitApiBaseUrl: string | undefined readonly defaultLocalApiBaseUrl: string | undefined readonly cachedApiBaseUrl: string | undefined readonly expectedRevision?: string | undefined -}): Effect.Effect => +} + +export const findReachableDirectHealthProbeWithHttpClient = ( + options: DirectHealthProbeOptions +): Effect.Effect => findReachableHealthProbeOrNull( buildApiBaseUrlCandidates({ explicitApiBaseUrl: options.explicitApiBaseUrl, @@ -166,10 +170,7 @@ export const findReachableDirectHealthProbeWithHttpClient = (options: { options.expectedRevision ) -export const findReachableDirectHealthProbe = (options: { - readonly explicitApiBaseUrl: string | undefined - readonly defaultLocalApiBaseUrl: string | undefined - readonly cachedApiBaseUrl: string | undefined - readonly expectedRevision?: string | undefined -}): Effect.Effect => +export const findReachableDirectHealthProbe = ( + options: DirectHealthProbeOptions +): Effect.Effect => findReachableDirectHealthProbeWithHttpClient(options).pipe(Effect.provide(FetchHttpClient.layer))