From 84d20e35a372e26d675a0ee312289fb1b26d2eaf Mon Sep 17 00:00:00 2001 From: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> Date: Mon, 21 Sep 2026 07:18:46 -0700 Subject: [PATCH] watch: prevent stale cross-board SVG from locking the wrong fit scale Motivation: Navigating between boards in `d2 --watch` (e.g. descending into a layer, then returning to the parent) is a full page load to a new URL path. watch.js computes a fit-to-window scale once, on the first websocket message received after a page load, and reuses it for every later message on that connection (intentional, so the viewport doesn't jump while live-editing a single board). The server hands a newly-connecting websocket client its single cached compile result immediately, without waiting for the navigation's own recompile to finish. If a compile for the previous board was still settling when the user navigated to a differently-sized board, the new page's first message could still carry the old board's SVG, locking in a scale fit to the wrong dimensions and never recomputing it once the correct SVG arrived. This is a cosmetic bug confined to the local, authenticated watch preview server. Approach: - handleRoot clears the shared cached result as soon as it detects the requested board differs from the one currently being viewed, before requesting a recompile, so a newly-connecting client gets nothing until the new board's compile actually finishes and broadcasts. - compileLoop captures which board it is compiling for and only broadcasts the result if that board is still the one being viewed. This closes a narrower race where a compile for the old board, still in flight at navigation time, could otherwise repopulate the cache with stale data right after the clear above. A fresh compile for the new board is always already queued, so dropping the stale broadcast is safe. Validation: - go build ./d2cli/... passes. - go vet ./d2cli/... and gofmt -l on the changed files report nothing. - ./make.sh lint (repo's fmt+vet target) passes, touching only these files. - go test ./d2cli/... -v -run "TestHandleRoot|TestBoardStillCurrent" passes for the three new/changed tests. - go test ./d2cli/... (full package) and ./ci/test.sh ./d2cli/... both pass. - Confirmed these are real regression tests: reverting only d2cli/watch.go makes TestHandleRootInvalidatesStaleResultOnBoardNavigation fail with a clear assertion, and makes the package fail to compile for TestBoardStillCurrentGuardsInFlightCompile (missing method) - then restoring the fix makes both pass again. - Did not run the full ./make.sh (JS/WASM tests, weekly TALA race suite, e2e release-archive build); this change is pure Go code in d2cli's HTTP/websocket handling and does not touch rendering, WASM, or the JS client beyond the pre-existing behavior described above. Report: https://github.com/d2lang/d2/issues/2931 Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> Assisted-by: claude-sonnet-5 (via Claude Code) --- d2cli/watch.go | 41 +++++++++++++++++++++++----- d2cli/watch_test.go | 66 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 100 insertions(+), 7 deletions(-) diff --git a/d2cli/watch.go b/d2cli/watch.go index ed8d2bb1f1..5045c7892f 100644 --- a/d2cli/watch.go +++ b/d2cli/watch.go @@ -347,6 +347,17 @@ func (w *watcher) requestCompile() { } } +// boardStillCurrent reports whether compiledFor is still the board being +// viewed. A compile can take long enough that the user navigates to a +// different board before it finishes; its result must be discarded rather +// than published, or it would overwrite the newly-selected board's cached +// result with stale data. +func (w *watcher) boardStillCurrent(compiledFor string) bool { + w.boardpathMu.Lock() + defer w.boardpathMu.Unlock() + return compiledFor == w.boardPath +} + func (w *watcher) ensureAddWatch(ctx context.Context, path string) (time.Time, error) { interval := time.Millisecond * 16 tc := time.NewTimer(0) @@ -449,9 +460,10 @@ func (w *watcher) compileLoop(ctx context.Context) error { // localfile.Policy to reject directories and other special files. fs := trackedFS{localFiles: localfile.Unrestricted()} w.boardpathMu.Lock() + compiledBoardPath := w.boardPath var boardPath []string - if w.boardPath != "" { - boardPath = strings.Split(w.boardPath, string(os.PathSeparator)) + if compiledBoardPath != "" { + boardPath = strings.Split(compiledBoardPath, string(os.PathSeparator)) } svg, _, err := compile(ctx, w.ms, &fs, w.layout, w.renderOpts, w.fontFamily, w.monoFontFamily, w.animateInterval, w.inputPath, w.outputPath, boardPath, false, w.bundle, w.forceAppendix, w.outputFormat, w.asciiMode, true) w.boardpathMu.Unlock() @@ -470,11 +482,17 @@ func (w *watcher) compileLoop(ctx context.Context) error { return err } - w.broadcast(&compileResult{ - SVG: string(svg), - Scale: w.renderOpts.Scale, - Err: errs, - }) + // handleRoot may have already switched to a different board and cleared + // w.res while this compile was in flight. Only publish this result if + // the board it was compiled for is still the one being viewed, so we + // never hand a newly-connecting client the wrong board's SVG. + if w.boardStillCurrent(compiledBoardPath) { + w.broadcast(&compileResult{ + SVG: string(svg), + Scale: w.renderOpts.Scale, + Err: errs, + }) + } if firstCompile { firstCompile = false @@ -611,6 +629,15 @@ func (w *watcher) handleRoot(hw http.ResponseWriter, r *http.Request) { } w.boardpathMu.Unlock() if recompile { + // Discard the previous board's cached result so a websocket client + // connecting for this navigation can't receive a stale SVG (with the + // wrong dimensions) before the new board finishes compiling. Without + // this, the client's watch.js locks in a fit-to-window scale computed + // from the old board and never recomputes it once the correct SVG + // arrives, leaving the diagram mis-scaled. + w.resMu.Lock() + w.res = nil + w.resMu.Unlock() w.requestCompile() } } diff --git a/d2cli/watch_test.go b/d2cli/watch_test.go index 5db74becce..627f125d34 100644 --- a/d2cli/watch_test.go +++ b/d2cli/watch_test.go @@ -2,6 +2,8 @@ package d2cli import ( "errors" + "net/http" + "net/http/httptest" "os" "path/filepath" "testing" @@ -65,3 +67,67 @@ func TestTrackedFSHonorsLocalFilePolicy(t *testing.T) { t.Fatalf("trusted tracked opens = %q, want [%q]", trusted.opened, outside) } } + +// TestHandleRootInvalidatesStaleResultOnBoardNavigation guards against a +// regression where navigating between boards in watch mode (e.g. descending +// into a layer, then returning to the parent board) could momentarily hand a +// newly-connecting websocket client the previous board's cached SVG. The +// frontend locks in its fit-to-window scale from the first message it +// receives, so a stale cross-board SVG left the diagram mis-scaled even +// after the correct one arrived. +func TestHandleRootInvalidatesStaleResultOnBoardNavigation(t *testing.T) { + w := &watcher{compileCh: make(chan struct{}, 1)} + w.boardPath = "old" + w.res = &compileResult{SVG: "old board"} + + req := httptest.NewRequest(http.MethodGet, "/new", nil) + w.handleRoot(httptest.NewRecorder(), req) + + if w.boardPath != "new" { + t.Fatalf("boardPath = %q, want %q", w.boardPath, "new") + } + if got := w.getRes(); got != nil { + t.Fatalf("getRes() = %+v, want nil after navigating to a different board", got) + } +} + +// TestHandleRootKeepsResultWhenBoardUnchanged ensures the invalidation above +// doesn't also drop the cached result on a plain reload of the same board, +// which would cause the diagram to flicker blank while it recompiles. +func TestHandleRootKeepsResultWhenBoardUnchanged(t *testing.T) { + w := &watcher{compileCh: make(chan struct{}, 1)} + w.boardPath = "same" + res := &compileResult{SVG: "same board"} + w.res = res + + req := httptest.NewRequest(http.MethodGet, "/same", nil) + w.handleRoot(httptest.NewRecorder(), req) + + if got := w.getRes(); got != res { + t.Fatalf("getRes() = %+v, want unchanged cached result %+v", got, res) + } +} + +// TestBoardStillCurrentGuardsInFlightCompile covers the remaining half of the +// stale-result race: a compile for the board being navigated away from can +// still be running when handleRoot clears w.res for the new board. Without +// this guard, compileLoop would broadcast that in-flight compile's result +// after the clear, repopulating w.res with the wrong board's SVG. +func TestBoardStillCurrentGuardsInFlightCompile(t *testing.T) { + w := &watcher{} + w.boardPath = "top" + + if !w.boardStillCurrent("top") { + t.Fatal("boardStillCurrent(\"top\") = false, want true before any navigation") + } + + // Simulate the user navigating away while a compile for "top" is in flight. + w.boardPath = "top.nested" + + if w.boardStillCurrent("top") { + t.Fatal("boardStillCurrent(\"top\") = true after navigating to \"top.nested\", want false") + } + if !w.boardStillCurrent("top.nested") { + t.Fatal("boardStillCurrent(\"top.nested\") = false, want true for the newly-selected board") + } +}