From ee429cfee796189297604da7774687605c0e04be Mon Sep 17 00:00:00 2001 From: Hongming Wang Date: Fri, 24 Apr 2026 22:23:51 -0700 Subject: [PATCH] fix(canvas,dotenv): review-driven hardening of fit gate + parser parity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Independent code review surfaced two required documentation fixes and one growth-correctness gap. All addressed here. Auto-fit gate (useCanvasViewport): The previous "subtree-grew-by-count" check missed the delete-then-add case: subtree of 6 → delete one → 5 → a different child arrives → 6 again. A length-only comparison reads no growth and the fit is skipped, leaving the new node off-screen. Switched to an id-set membership snapshot so any brand-new id forces the fit even when the count is unchanged. The gate logic is now extracted as a pure exported function `shouldFitGrowing(currentIds, prevIds, userPannedAt, lastAutoFitAt)` so the regression-prone decision can be unit-tested in isolation without standing up React Flow + DOM event refs. 8 cases cover: first-fit, empty-prior, brand-new id, status-update with user pan, no-pan-ever, pan-before-last-fit, delete-then-add same length, and shrink-only with user pan. Parser parity (dotenv.go + next.config.ts): Existing-env semantics were undocumented in both parsers. Both now explicitly note that an explicitly-set empty string (`KEY=` from the parent shell) counts as "set" — the file value does NOT backfill — matching the Go (os.LookupEnv) and Node (`process.env[k] !== undefined`) primitives. `export ` prefix uses a literal space; `export\tFOO=bar` is intentionally rejected. Added the same comment in both parsers to lock in this parity invariant since the commit message claims "if one parser changes, the other has to." Skipped (per analysis): - Drag-pan respect for left-click drag-pan during deploy. The growth-check safety net means any pan gets overridden on the next arrival anyway, which is the desired behavior for the "watch the org deploy" use case. After deploy completes, no more fit-deploying-org events fire so drag-pan works freely. - Map cleanup for lastFitSubtreeIdsRef. Per-tab session, UUID keys, tiny entries — not worth the cleanup hook. 993 canvas tests pass (8 new); Go dotenv tests pass; tsc clean. Co-Authored-By: Claude Opus 4.7 (1M context) --- canvas/next.config.ts | 12 +++ .../__tests__/useCanvasViewport.test.ts | 53 ++++++++++++ .../components/canvas/useCanvasViewport.ts | 81 ++++++++++++++----- workspace-server/cmd/server/dotenv.go | 16 +++- 4 files changed, 138 insertions(+), 24 deletions(-) create mode 100644 canvas/src/components/canvas/__tests__/useCanvasViewport.test.ts 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, '=')