Skip to content

watch: prevent stale cross-board SVG from locking the wrong fit scale - #2934

Open
pujitha24 wants to merge 1 commit into
d2lang:masterfrom
pujitha24:auto/issue-2931
Open

pujitha24 wants to merge 1 commit into
d2lang:masterfrom
pujitha24:auto/issue-2931

Conversation

@pujitha24

Copy link
Copy Markdown

Motivation

In d2 --watch, navigating between boards (e.g. descending into a layer, then
returning to the parent) is a full browser page load to a new URL path.
watch.js computes a fit-to-window scale once, on the first websocket
message it receives after a page load, and reuses that scale for every later
message on the same connection — this is intentional, so the viewport
doesn't jump around while a user is live-editing a single board.

The server keeps one shared w.res (cached compile result) and hands it to
a newly-connecting websocket client immediately, before waiting for the
current navigation's own compile to finish. If a compile for the old
board was still in flight (or its result hadn't been superseded yet) when
the user navigated to a very differently-sized board, the new page's first
websocket message could still carry the old board's SVG. Since that's the
first message on the fresh connection, the client locks in a scale fit to
the wrong board's dimensions, and never recomputes it once the correct SVG
arrives — leaving the newly-navigated-to board mis-scaled.

This is a cosmetic bug confined to the local, authenticated d2 --watch
preview server; nothing about compiled output, security, or data integrity
is affected.

Approach

Two changes in d2cli/watch.go, both gated on an actual board-path change
(not a same-board reload or ordinary live-edit recompile, to avoid any new
blank-flash regression):

  1. handleRoot clears the shared w.res to nil as soon as it detects the
    requested board differs from the one currently being viewed, before
    kicking off the recompile. This closes the common case: a
    newly-connecting websocket client gets nothing until the new board's
    compile actually finishes and broadcasts.
  2. compileLoop captures which board path it's compiling for, and after the
    compile finishes, only broadcasts the result if that board path is still
    the one being viewed (boardStillCurrent). This closes a narrower race:
    a compile for the old board that was still in flight when the
    navigation happened could otherwise re-populate w.res with stale data
    immediately after step 1's clear, since handleRoot only waits on a
    mutex the in-flight compile releases before it finishes broadcasting, not
    on the broadcast itself. A fresh compile for the new board is already
    queued by handleRoot, so silently dropping the stale one's broadcast is
    safe — the diagram never gets stuck.

Validation

  • go build ./d2cli/... — passes.
  • go vet ./d2cli/... and gofmt -l d2cli/watch.go d2cli/watch_test.go — clean.
  • ./make.sh lint (repo's fmt+vet target) — passes, touching only these two files.
  • go test ./d2cli/... -v -run "TestHandleRoot|TestBoardStillCurrent" — the
    three new/changed tests pass:
    • TestHandleRootInvalidatesStaleResultOnBoardNavigation
    • TestHandleRootKeepsResultWhenBoardUnchanged
    • TestBoardStillCurrentGuardsInFlightCompile
  • go test ./d2cli/... (full package) and ./ci/test.sh ./d2cli/... (repo's
    documented test runner) — both pass.
  • Confirmed these are real regression tests, not tautological: reverting
    only d2cli/watch.go (git stash) makes
    TestHandleRootInvalidatesStaleResultOnBoardNavigation fail with a clear
    assertion, and makes the package fail to compile for
    TestBoardStillCurrentGuardsInFlightCompile (boardStillCurrent
    undefined) — then restored the fix and reconfirmed both pass.

I did not run the full ./make.sh (JS/WASM tests, weekly TALA race suite,
e2e release-archive build) since this change is pure Go code inside
d2cli's HTTP/websocket handling and doesn't touch rendering, WASM, or the
JS client beyond the pre-existing behavior described above. I also did not
add a ci/release/changelogs/next.md entry, since those entries link the
PR number, which isn't known until this PR is opened.

Report: #2931

Fixes #2931

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: d2lang#2931
Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
Assisted-by: claude-sonnet-5 (via Claude Code)

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Watch viewer does not scale properly

1 participant