diff --git a/canvas/next.config.ts b/canvas/next.config.ts index 08c559cf..079e21c2 100644 --- a/canvas/next.config.ts +++ b/canvas/next.config.ts @@ -36,6 +36,14 @@ function loadMonorepoEnv() { const kv = parseLine(line); if (!kv) continue; const [k, v] = kv; + // Existing env wins. NOTE: an explicitly-set empty string + // (`KEY=` exported from a parent shell, where Node represents it + // as `""` not `undefined`) counts as "set" — we keep the empty + // value rather than backfilling from the file. Matches Go's + // os.LookupEnv check in workspace-server/cmd/server/dotenv.go so + // both processes treat the same input identically. Operators who + // want the file value to win must `unset KEY` in the launching + // shell. if (process.env[k] !== undefined) { skipped++; continue; @@ -66,6 +74,10 @@ function findMonorepoRoot(start: string): string | null { function parseLine(raw: string): [string, string] | null { let line = raw.replace(/^/, "").trim(); if (line === "" || line.startsWith("#")) return null; + // `export ` prefix uses a literal space — `export\tFOO=bar` with a + // tab is intentionally rejected, matching the Go mirror in + // workspace-server/cmd/server/dotenv.go. Shells emit the prefix + // with a space; tabs would only appear in hand-mangled files. if (line.startsWith("export ")) line = line.slice("export ".length).trimStart(); const eq = line.indexOf("="); if (eq <= 0) return null; diff --git a/canvas/src/components/canvas/__tests__/useCanvasViewport.test.ts b/canvas/src/components/canvas/__tests__/useCanvasViewport.test.ts new file mode 100644 index 00000000..4d21ea91 --- /dev/null +++ b/canvas/src/components/canvas/__tests__/useCanvasViewport.test.ts @@ -0,0 +1,53 @@ +import { describe, it, expect } from "vitest"; +import { shouldFitGrowing } from "../useCanvasViewport"; + +// Tests cover the auto-fit gate in isolation. The hook itself is +// effects + refs + React Flow handles, awkward to exercise directly — +// extracting the pure decision into shouldFitGrowing(...) lets us +// pin down the regression-prone logic with unit tests instead. + +describe("shouldFitGrowing", () => { + it("fits the very first time (no prior snapshot)", () => { + expect(shouldFitGrowing(["a"], undefined, null, 0)).toBe(true); + }); + + it("fits when the prior snapshot is empty", () => { + expect(shouldFitGrowing(["a", "b"], new Set(), null, 0)).toBe(true); + }); + + it("fits when a brand-new id has been added since the last fit", () => { + const prev = new Set(["root", "a", "b"]); + expect(shouldFitGrowing(["root", "a", "b", "c"], prev, null, 0)).toBe(true); + }); + + it("respects user pan when the subtree hasn't grown", () => { + const prev = new Set(["root", "a", "b"]); + // Status update on existing node — same membership. + expect(shouldFitGrowing(["root", "a", "b"], prev, 5_000, 1_000)).toBe(false); + }); + + it("fits when the subtree hasn't grown but the user never panned", () => { + const prev = new Set(["root", "a", "b"]); + expect(shouldFitGrowing(["root", "a", "b"], prev, null, 1_000)).toBe(true); + }); + + it("fits when the subtree hasn't grown and the user panned BEFORE the last fit", () => { + const prev = new Set(["root", "a", "b"]); + expect(shouldFitGrowing(["root", "a", "b"], prev, 500, 1_000)).toBe(true); + }); + + it("forces fit on delete-then-add even when the count is unchanged", () => { + // Subtree was [root, a, b, c, d]. Then `d` got removed and a + // sibling `e` arrived. Same length, different membership — a + // length-only check would skip the fit and leave `e` off-screen. + const prev = new Set(["root", "a", "b", "c", "d"]); + expect( + shouldFitGrowing(["root", "a", "b", "c", "e"], prev, 5_000, 1_000), + ).toBe(true); + }); + + it("does NOT fit on shrink-only when the user has panned (deletion alone shouldn't override exploration)", () => { + const prev = new Set(["root", "a", "b", "c"]); + expect(shouldFitGrowing(["root", "a", "b"], prev, 5_000, 1_000)).toBe(false); + }); +}); diff --git a/canvas/src/components/canvas/useCanvasViewport.ts b/canvas/src/components/canvas/useCanvasViewport.ts index 1a528b89..75ddf7bd 100644 --- a/canvas/src/components/canvas/useCanvasViewport.ts +++ b/canvas/src/components/canvas/useCanvasViewport.ts @@ -9,6 +9,37 @@ import { CHILD_DEFAULT_WIDTH, } from "@/store/canvas-topology"; +/** + * Decide whether the deploy-time auto-fit should run. Pure function so + * the gate logic is unit-testable in isolation — the surrounding + * useEffect tangle of refs, timers, and React Flow handles is awkward + * to exercise directly. + * + * Returns true when the auto-fit SHOULD fire: + * - the subtree contains an id that wasn't in the previous snapshot + * (a new node arrived → user has lost context, force the fit + * through regardless of any user-pan in between), OR + * - the user has not panned since the last successful fit (so the + * auto-fit isn't fighting their override). + * + * `prevSubtreeIds === undefined` means no fit has ever run for this + * root — treat every id as "new" and fit. `userPannedAt === null` + * means the user has never panned at all in this session — fit. + */ +export function shouldFitGrowing( + currentSubtreeIds: readonly string[], + prevSubtreeIds: ReadonlySet | undefined, + userPannedAt: number | null, + lastAutoFitAt: number, +): boolean { + if (!prevSubtreeIds || prevSubtreeIds.size === 0) return true; + for (const id of currentSubtreeIds) { + if (!prevSubtreeIds.has(id)) return true; + } + if (userPannedAt === null) return true; + return userPannedAt <= lastAutoFitAt; +} + /** * Wires the two canvas-wide CustomEvent listeners and the viewport * save/restore bookkeeping so Canvas.tsx doesn't have to. @@ -194,14 +225,21 @@ export function useCanvasViewport() { // circuits: if the user moved after our last auto-fit, we never // fit again this deploy. const pendingFitRootRef = useRef(null); - // Subtree size at the moment of the last successful auto-fit, keyed - // by root id. When a new event arrives and the subtree has GROWN, - // we re-fit even if the user has panned in the meantime — they've - // lost context, the new arrivals are off-screen, and the org-deploy - // animation is the user's primary reason for being on the canvas - // right now. When the subtree hasn't grown (status update on an - // already-positioned node), the user-pan respect still applies. - const lastFitSubtreeSizeRef = useRef>(new Map()); + // Membership snapshot of the subtree at the moment of the last + // successful auto-fit, keyed by root id. When a new event arrives, + // we compute growth as "any id in the current subtree that wasn't + // in the snapshot". An id-set rather than just a count handles the + // delete-then-add case correctly: subtree of 6 → delete one → 5 → + // a different child arrives → 6 again. A length-only comparison + // would call this "no growth" and skip the fit even though a + // brand-new node landed off-screen. The id-set sees the new id + // wasn't in the snapshot and forces the fit. + // + // Map is keyed by root id and never pruned. Acceptable today because + // org roots are UUIDs (no collisions on retry / template re-import), + // canvas sessions are per-tab, and entries are tiny. Worth a sweep + // if long-lived sessions ever start importing hundreds of orgs. + const lastFitSubtreeIdsRef = useRef>>(new Map()); useEffect(() => { const runFit = () => { const rootCandidate = pendingFitRootRef.current; @@ -229,18 +267,19 @@ export function useCanvasViewport() { } if (subtree.length === 0) return; - // Growth check: if the subtree hasn't expanded since the last - // fit, fall back to the user-pan respect gate. If it HAS - // grown, force the fit through — the user's previous pan no - // longer reflects what's on screen and the deploy is the - // primary thing they want to watch. - const lastSize = lastFitSubtreeSizeRef.current.get(topId) ?? 0; - const grew = subtree.length > lastSize; - if ( - !grew && - userPannedAtRef.current !== null && - userPannedAtRef.current > lastAutoFitAtRef.current - ) { + // Growth check: did any id in the current subtree NOT appear + // in the snapshot from the last fit? If yes, fit through + // regardless of the user-pan timestamp — the user has lost + // context, the new arrival is off-screen, and the deploy is + // the primary thing they want to watch. If no, fall back to + // the user-pan respect gate so post-deploy exploration isn't + // yanked back. + if (!shouldFitGrowing( + subtree, + lastFitSubtreeIdsRef.current.get(topId), + userPannedAtRef.current, + lastAutoFitAtRef.current, + )) { return; } fitView({ @@ -266,7 +305,7 @@ export function useCanvasViewport() { minZoom: 0.25, }); lastAutoFitAtRef.current = Date.now(); - lastFitSubtreeSizeRef.current.set(topId, subtree.length); + lastFitSubtreeIdsRef.current.set(topId, new Set(subtree)); }; const handler = (e: Event) => { const { rootId } = (e as CustomEvent<{ rootId: string }>).detail; diff --git a/workspace-server/cmd/server/dotenv.go b/workspace-server/cmd/server/dotenv.go index 02d504a5..fd5745ce 100644 --- a/workspace-server/cmd/server/dotenv.go +++ b/workspace-server/cmd/server/dotenv.go @@ -52,6 +52,13 @@ func loadDotEnvIfPresent() { if !ok { continue } + // Existing env wins. NOTE: an explicitly-set empty string + // (`KEY=` exported from a parent shell) counts as "set" — we + // keep the empty value rather than backfilling from the file. + // Matches Node's `process.env[k] !== undefined` check in the + // canvas's next.config.ts loader so both processes treat the + // same input identically. Operators who want the file value + // to win must `unset KEY` in the launching shell. if _, exists := os.LookupEnv(k); exists { skipped++ continue @@ -133,9 +140,12 @@ func parseDotEnvLine(line string) (string, string, bool) { if line == "" || strings.HasPrefix(line, "#") { return "", "", false } - // Drop a leading `export ` so lines like `export FOO=bar` (the - // form direnv and many `.env` templates emit) don't end up as a - // junk key with an embedded space. + // Drop a leading `export ` (literal space — `export\tFOO=bar` + // with a tab is intentionally rejected, matching the TS mirror in + // canvas/next.config.ts. shells emit `export ` with a space; tabs + // would only appear in hand-mangled files.) so lines like + // `export FOO=bar` (the form direnv and many `.env` templates + // emit) don't end up as a junk key with an embedded space. line = strings.TrimPrefix(line, "export ") line = strings.TrimLeft(line, " \t") // re-trim in case `export` itself had trailing space eq := strings.IndexByte(line, '=')