From 79b091b035ff89c26ddd6dc9be9aa5b68b6d4441 Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Fri, 16 Oct 2026 14:02:44 -0400 Subject: [PATCH] Preserve environment resources through editor saves --- .../app/components/environment-form.test.ts | 117 ++++++++++++++ .../app/components/environment-form.tsx | 148 +++++------------- 2 files changed, 159 insertions(+), 106 deletions(-) diff --git a/apps/fabro-web/app/components/environment-form.test.ts b/apps/fabro-web/app/components/environment-form.test.ts index 0b661278c..4ef240c43 100644 --- a/apps/fabro-web/app/components/environment-form.test.ts +++ b/apps/fabro-web/app/components/environment-form.test.ts @@ -1,9 +1,16 @@ import { describe, expect, test } from "bun:test"; +import { createElement, useState } from "react"; +import TestRenderer, { act } from "react-test-renderer"; +import type { Environment, EnvironmentResourcesSettings } from "@qltysh/fabro-api-client"; +import { setupReactTestEnv } from "../lib/test-utils"; import { EMPTY_ENVIRONMENT_FORM, + EnvironmentFormFields, createRequestFromForm, + environmentToFormValues, isEnvironmentFormValid, + replaceRequestFromForm, type EnvironmentFormValues, } from "./environment-form"; @@ -48,3 +55,113 @@ describe("environment image source", () => { expect(request.image.dockerfile?.value).toBe("FROM ubuntu"); }); }); + +function environment(resources: EnvironmentResourcesSettings): Environment { + return { + id: "roundtrip", + revision: "revision-1", + provider: "docker", + image: { docker: "ubuntu:24.04", dockerfile: null }, + resources, + network: { mode: "allow_all", allow: [] }, + lifecycle: { preserve: false, stop_on_terminal: true, auto_stop: null }, + labels: { owner: "api", purpose: "roundtrip" }, + env: { TZ: "UTC" }, + }; +} + +describe("environment resource round trips", () => { + const cases: EnvironmentResourcesSettings[] = [ + // API response for 512MiB; regression originally reported in #952. + { cpu: null, memory: "536870912B", disk: null }, + { cpu: 4, memory: "4GB", disk: "8GB" }, + { cpu: null, memory: null, disk: null }, + { cpu: 32, memory: "64GB", disk: "2TB" }, + { cpu: 1, memory: "512MiB", disk: "1GiB" }, + { cpu: 2, memory: "1.5GB", disk: "512MB" }, + { cpu: 2, memory: "1B", disk: "1TiB" }, + ]; + + for (const resources of cases) { + test(`no-op save preserves ${JSON.stringify(resources)}`, () => { + const original = environment(resources); + const { id, revision, ...settings } = original; + expect(replaceRequestFromForm(environmentToFormValues(original))).toEqual(settings); + }); + + test(`unrelated edits preserve ${JSON.stringify(resources)} and labels`, () => { + const original = environment(resources); + const values = environmentToFormValues(original); + const saved = replaceRequestFromForm({ + ...values, + dockerRef: "ubuntu:26.04", + preserve: true, + autoStop: "30m", + envVars: [...values.envVars, { key: "EDITOR", value: "vi" }], + }); + expect(saved.resources).toEqual(resources); + expect(saved.labels).toEqual(original.labels); + expect(saved.image.docker).toBe("ubuntu:26.04"); + expect(saved.lifecycle).toEqual({ preserve: true, stop_on_terminal: true, auto_stop: "30m" }); + expect(saved.env).toEqual({ TZ: "UTC", EDITOR: "vi" }); + }); + } + + test("editing one resource preserves the other resources", () => { + const values = environmentToFormValues(environment(cases[0])); + expect(replaceRequestFromForm({ ...values, cpu: 16 }).resources).toEqual({ + ...cases[0], cpu: 16, + }); + expect(replaceRequestFromForm({ ...values, memory: "768MiB" }).resources).toEqual({ + ...cases[0], memory: "768MiB", + }); + expect(replaceRequestFromForm({ ...values, disk: "32GB" }).resources).toEqual({ + ...cases[0], disk: "32GB", + }); + }); + + test("clearing resources restores provider defaults", () => { + const values = environmentToFormValues(environment(cases[1])); + expect(replaceRequestFromForm({ ...values, cpu: null, memory: "", disk: " " }).resources) + .toEqual({ cpu: null, memory: null, disk: null }); + }); + + test("new environments keep the existing resource defaults", () => { + expect(createRequestFromForm(form({ dockerRef: "ubuntu:24.04" })).resources) + .toEqual({ cpu: 4, memory: "8GB", disk: "16GB" }); + }); +}); + +test("resource inputs display API values and preserve them through user edits", () => { + const cleanup = setupReactTestEnv(); + let renderer: TestRenderer.ReactTestRenderer | undefined; + let saved: ReturnType | undefined; + function Editor() { + const [values, setValues] = useState(() => environmentToFormValues(environment({ + cpu: null, memory: "536870912B", disk: null, + }))); + saved = replaceRequestFromForm(values); + return createElement(EnvironmentFormFields, { values, onChange: setValues, lockId: true }); + } + try { + act(() => { renderer = TestRenderer.create(createElement(Editor)); }); + const input = (label: string) => renderer!.root.findByProps({ "aria-label": label }); + expect(input("CPU").props.value).toBe(""); + expect(input("Memory").props.value).toBe("536870912B"); + expect(input("Disk").props.value).toBe(""); + act(() => { input("Image reference").props.onChange({ target: { value: "alpine:latest" } }); }); + expect(saved!.resources).toEqual({ cpu: null, memory: "536870912B", disk: null }); + expect(saved!.labels).toEqual({ owner: "api", purpose: "roundtrip" }); + act(() => { input("CPU").props.onChange({ target: { value: "32" } }); }); + act(() => { input("Memory").props.onChange({ target: { value: "768MiB" } }); }); + act(() => { input("Disk").props.onChange({ target: { value: "64GB" } }); }); + expect(saved!.resources).toEqual({ cpu: 32, memory: "768MiB", disk: "64GB" }); + for (const label of ["CPU", "Memory", "Disk"]) { + act(() => { input(label).props.onChange({ target: { value: "" } }); }); + } + expect(saved!.resources).toEqual({ cpu: null, memory: null, disk: null }); + } finally { + act(() => { renderer?.unmount(); }); + cleanup(); + } +}); diff --git a/apps/fabro-web/app/components/environment-form.tsx b/apps/fabro-web/app/components/environment-form.tsx index a78a6cc10..b8f8ba542 100644 --- a/apps/fabro-web/app/components/environment-form.tsx +++ b/apps/fabro-web/app/components/environment-form.tsx @@ -39,12 +39,6 @@ export function parseCreatableProvider(value: string | null): string { // Environment ids are server-managed file names: lowercase, digits, hyphens. const ENVIRONMENT_ID_PATTERN = /^[a-z0-9][a-z0-9-]{0,62}$/; -// Resource sliders pick a concrete value within a fixed range. Memory and disk -// are expressed in whole GB; the wire format keeps the `GB` suffix string. -const CPU = { min: 1, max: 8, step: 1, default: 4 }; -const MEMORY = { min: 1, max: 16, step: 1, default: 8 }; -const DISK = { min: 1, max: 20, step: 1, default: 16 }; - // An environment image comes from exactly one source: a prebuilt image // reference or an inline Dockerfile. The form keeps both field values around so // switching back and forth doesn't lose typed text, and this discriminator @@ -57,9 +51,10 @@ export interface EnvironmentFormValues { imageSource: ImageSource; dockerRef: string; dockerfile: string; - cpu: number; - memory: number; - disk: number; + // Keep API resource values intact; blank size inputs represent provider defaults. + cpu: number | null; + memory: string; + disk: string; blockNetwork: boolean; preserve: boolean; stopOnTerminal: boolean; @@ -77,9 +72,9 @@ export const EMPTY_ENVIRONMENT_FORM: EnvironmentFormValues = { imageSource: "image", dockerRef: "", dockerfile: "", - cpu: CPU.default, - memory: MEMORY.default, - disk: DISK.default, + cpu: 4, + memory: "8GB", + disk: "16GB", blockNetwork: false, preserve: false, stopOnTerminal: true, @@ -95,9 +90,9 @@ export function environmentToFormValues(environment: Environment): EnvironmentFo imageSource: environment.image.dockerfile ? "dockerfile" : "image", dockerRef: environment.image.docker ?? "", dockerfile: environment.image.dockerfile?.value ?? "", - cpu: clampGb(environment.resources.cpu, CPU), - memory: parseGb(environment.resources.memory, MEMORY), - disk: parseGb(environment.resources.disk, DISK), + cpu: environment.resources.cpu, + memory: environment.resources.memory ?? "", + disk: environment.resources.disk ?? "", blockNetwork: environment.network.mode === EnvironmentNetworkMode.BLOCK, preserve: environment.lifecycle.preserve, stopOnTerminal: environment.lifecycle.stop_on_terminal, @@ -169,43 +164,11 @@ function imageFromForm(values: EnvironmentFormValues): EnvironmentApiImageSettin function resourcesFromForm(values: EnvironmentFormValues): EnvironmentResourcesSettings { return { cpu: values.cpu, - memory: `${values.memory}GB`, - disk: `${values.disk}GB`, + memory: values.memory.trim() || null, + disk: values.disk.trim() || null, }; } -interface ResourceRange { - min: number; - max: number; - step: number; - default: number; -} - -// Snap a numeric value into the slider range, falling back to the default when -// the environment leaves the resource unset (provider default). -function clampGb(value: number | null, range: ResourceRange): number { - if (value === null) return range.default; - return Math.min(range.max, Math.max(range.min, Math.round(value))); -} - -// Parse a size string ("16GB", "512MiB", or a bare integer interpreted as GB) -// into whole GB within the slider range. Existing values may use other units or -// fall outside the range, so the result is rounded and clamped. -function parseGb(value: string | null, range: ResourceRange): number { - if (value === null) return range.default; - const match = value.trim().match(/^([\d.]+)\s*([a-zA-Z]*)$/); - if (!match) return range.default; - const amount = Number(match[1]); - if (!Number.isFinite(amount)) return range.default; - const perGb: { [unit: string]: number } = { - "": 1, g: 1, gb: 1, gib: 1, - m: 1 / 1000, mb: 1 / 1000, mib: 1 / 1000, - t: 1000, tb: 1000, tib: 1000, - }; - const factor = perGb[match[2].toLowerCase()] ?? 1; - return clampGb(amount * factor, range); -} - function networkFromForm(values: EnvironmentFormValues): EnvironmentNetworkSettings { return { mode: values.blockNetwork ? EnvironmentNetworkMode.BLOCK : EnvironmentNetworkMode.ALLOW_ALL, @@ -318,31 +281,42 @@ export function EnvironmentFormFields({ - - patch({ cpu })} - format={(n) => `${n} CPU`} + CPU} help="Number of vCPUs allocated to each run. Leave blank for the provider default."> + patch({ cpu: e.target.value === "" ? null : Number(e.target.value) })} + placeholder="Provider default" + className={`${INPUT_CLASS} font-mono`} /> - - Memory} help="Memory limit, including units (e.g. 512MiB or 4GB). Leave blank for the provider default."> + patch({ memory })} - format={(n) => `${n} GB`} + onChange={(e) => patch({ memory: e.target.value })} + placeholder="Provider default" + autoComplete="off" + spellCheck={false} + className={`${INPUT_CLASS} font-mono`} /> - - Disk} help="Disk limit, including units (e.g. 8GB). Leave blank for the provider default."> + patch({ disk })} - format={(n) => `${n} GB`} + onChange={(e) => patch({ disk: e.target.value })} + placeholder="Provider default" + autoComplete="off" + spellCheck={false} + className={`${INPUT_CLASS} font-mono`} /> @@ -425,44 +399,6 @@ export function EnvironmentFormFields({ ); } -function ResourceSlider({ - value, - range, - ariaLabel, - format, - onChange, -}: { - value: number; - range: ResourceRange; - ariaLabel: string; - format: (value: number) => string; - onChange: (value: number) => void; -}) { - const fill = ((value - range.min) / (range.max - range.min)) * 100; - return ( -
-
-
-
-
- onChange(Number(e.target.value))} - className="relative h-4 w-full cursor-pointer appearance-none bg-transparent focus-visible:outline-none [&::-moz-range-thumb]:size-4 [&::-moz-range-thumb]:rounded-full [&::-moz-range-thumb]:border-0 [&::-moz-range-thumb]:bg-fg [&::-moz-range-thumb]:shadow-sm [&::-moz-range-track]:h-1.5 [&::-moz-range-track]:rounded-full [&::-moz-range-track]:bg-transparent [&::-webkit-slider-runnable-track]:h-1.5 [&::-webkit-slider-runnable-track]:rounded-full [&::-webkit-slider-runnable-track]:bg-transparent [&::-webkit-slider-thumb]:-mt-[5px] [&::-webkit-slider-thumb]:size-4 [&::-webkit-slider-thumb]:appearance-none [&::-webkit-slider-thumb]:rounded-full [&::-webkit-slider-thumb]:bg-fg [&::-webkit-slider-thumb]:shadow-sm [&::-webkit-slider-thumb]:outline [&::-webkit-slider-thumb]:outline-1 [&::-webkit-slider-thumb]:-outline-offset-1 [&::-webkit-slider-thumb]:outline-line-strong focus-visible:[&::-webkit-slider-thumb]:outline-2 focus-visible:[&::-webkit-slider-thumb]:outline-teal-500" - /> -
- - {format(value)} - -
- ); -} - function ToggleSwitch({ checked, onChange,