Tree svg scales incorrectly when window is resized (e.g. by opening dev console)

[corneliusroemer]

On next.nextstrain.org (v2.61.1) I noticed that the tree panel scales incorrectly and ends up in a wrongly scaled state when resizing the window, e.g. by opening the dev console. See GIF for details:

[jameshadfield]

Bisecting points to

3a18d773adedafe6f41fce26fa7d814ceeec395f is the first bad commit
commit 3a18d773adedafe6f41fce26fa7d814ceeec395f
Author: james hadfield
Date:   Tue Oct 29 10:04:38 2024 +1300

    Skip (some) tree animations for big trees

[jameshadfield]

Debugging one situation where this happens on a <4k tip tree leads me to suspect that it's the same underlying bug as #1903. It involves complex interactions of multiple d3 updates. Focusing on a single tip + resizing the window:

1. Resizing the window triggers a number of calls to update the tree in quick succession (aside: this is poor, we should debounce it and only trigger on the trailing edge).
2. The transitionTime is initially 500 (ms) and subsequently 0 (ms) due to the frequency of resize actions. (This behaviour isn't new.)
3. We are correctly calculating the intended new position of elements (wrt to the SVG size)
4. The d3 update call, when console.logged indicates we're changing the position (attrs) of elements (e.g. tips) to their correct location. The details matter here:
   4a. If the transitionTime is 0ms we don't set a transition on the d3 update call (here "transition" means transition().duration(transitionTime))
   4b. If transitionTime>0 then we set a transition on the d3 update call.
   4c. The previous behaviour was to always set transitions, and if transitionTime===0 then we'd immediately call timerFlush
5. When the bug occurs the attr on the underlying DOM element represents a value set by a/the previous d3 update call.

The bug can be fixed by either using the previous implementation of always calling d3 updates using a transition (step 4c) or by always setting the transitionTime to 0m (step 2) and thus never using a transition. So I suspect the bug is along the lines of:

• Initial update is on a 500ms transition and thus involves setTimeout/requestAnimationFrame callbacks
• Subsequent updates are immediate (no transitions) and go through
• The callbacks from the initial update run and we are out-of-sync

[jameshadfield]

This situation (an update with a 500ms duration "interrupted" by another action) is occurring a lot - window resizes, date slider changes, mousing over legend swatches, even quick successive changes to color-by. I'm fairly confident now that it's behind #1903.

The ideal solution would be to detect if any d3 updates are in-progress and, if so, call timerFlush before making another update. In the meantime i'll switch back to our previous implementation (step 4c).

[jameshadfield]

The ideal solution would be to detect if any d3 updates are in-progress and, if so, call timerFlush before making another update.

I think the following essentially achieves this, but would need a lot more testing (and shouldn't prevent us merging #1907 wile doing so)

diff --git a/src/components/tree/phyloTree/change.ts b/src/components/tree/phyloTree/change.ts
index 0c586f7d..47f94ecc 100644
--- a/src/components/tree/phyloTree/change.ts
+++ b/src/components/tree/phyloTree/change.ts
@@ -280,6 +280,8 @@ interface Extras {
 }
 
 
+let lastCallInvolvedATransition;
+
 /* the main interface to changing a currently rendered tree.
  * simply call change and tell it what should be changed.
  * try to do a single change() call with as many things as possible in it
@@ -328,6 +330,14 @@ export const change = function change(
   if ((Date.now() - this.timeLastRenderRequested) < idealTransitionTime * 2 || performanceFlags.get("skipTreeAnimation")===true) {
     transitionTime = 0;
   }
+  if (lastCallInvolvedATransition) {
+    // ideally we'd know if any d3 timers were ongoing (stored privately by d3's `taskHead` I think)
+    // and only call timerFlush if there are any. But looking at the code of `timerFlush` I think it should
+    // be very efficient if there are no timers in play.
+    timerFlush();
+  }
+  if (transitionTime) lastCallInvolvedATransition = true;
+
 
   /* the logic of converting what react is telling us to change
   and what SVG elements, node properties, svg props we actually change */

[corneliusroemer]

Thanks for the investigation! I don't know enough about d3 to be helpful on the technical side unfortunately. Could we just have a global "transition off" switch as a fallback for large trees and/or slow devices or when people don't want transitions? I know this adds yet another customization but it might help discover how important transitions are, whether they are worth the performance hit, bugs etc