Fix zoom freezing tips whose radius changed mid-transition#2077
Open
trvrb wants to merge 21 commits into
Open
Conversation
The previous interface was slightly cumbersome - I often found myself wanting to source both datasets & narratives from some directory X and having to type in the rather long `--datasetDir X --narrativeDir X`, and often I'd misspell / pluralise the argument names; using a simple positional argument `X` is much nicer. The ability to use multiple directories is also functionality I've long wanted. The previous CLI interface is preserved in its entirety for backwards compatibility and is mutually exclusive with the new positional-args style.
Stands up the default getDataset handler on an ephemeral port and makes real HTTP requests against the JSONs in test/fetched-jsons, covering both the main (v2) dataset and the tip-frequencies sidecar. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Convert all server-side code (cli/, auspice.js, index.js, src/version.js) and the test file from CommonJS require/module.exports syntax to ES module import/export syntax. This also addresses the downstream effects on webpack bundling for the frontend code in src/. package.json: Added "type": "module" to declare the package as ESM. This makes Node treat all .js files as ES modules by default. Replaced the "main" field with "exports" to use the modern package entry point specification. Added babel-plugin-transform-import-meta as a dev dependency for Jest compatibility. Server code (cli/, auspice.js, index.js, src/version.js): Converted all require() calls to import statements and all module.exports/exports assignments to named or default exports. For packages that use CommonJS internally (argparse, webpack config), createRequire(import.meta.url) is used to obtain a require function that can load CJS modules from an ESM context. __dirname shim: Since __dirname is not available in ES modules, files that reference it (cli/utils.js, cli/build.js, cli/develop.js, cli/view.js) now derive it via path.dirname(fileURLToPath(import.meta.url)). The test file also uses this pattern. Dynamic require(handlersPath) in cli/view.js: The user-provided custom handlers path was previously loaded with require(). This is now await import(handlersPath), which required making customRouteHandlers() and run() async functions. Config file renames: webpack.config.js and babel.config.js are renamed to .cjs since they use CommonJS syntax (module.exports) and would otherwise be treated as ESM due to the package-level "type": "module" setting. Babel transform for Jest: Jest runs tests through babel-jest which transforms ESM to CJS. However, babel leaves import.meta.url untouched, causing a runtime error when Jest executes the transformed code. Added babel-plugin-transform-import-meta (test env only) to convert import.meta.url to its CJS equivalent during testing. The test/server-fetch.test.js file itself was also converted to ESM syntax. webpack.config.cjs updates: Added resolve.fullySpecified: false (both top-level and as a per-module rule) to allow extensionless imports in the bundled frontend code -- without this, webpack enforces mandatory file extensions for all imports in "type": "module" packages. The JS/TS rule also sets type: "javascript/auto" so webpack parses source files in mixed mode, recognizing both require() and import syntax. module.hot to import.meta.webpackHot: In src/root.js and src/store.ts, the webpack HMR API was accessed via module.hot which is a CJS-only global. Replaced with import.meta.webpackHot, the ESM equivalent. The HMR accept callback in store.ts now uses dynamic import() instead of require() to reload reducers. Frontend require() calls for assets: Several React components used require() to load images (SVG, PNG, JPG). These are converted to static import statements at the top of each file, which webpack asset/resource loader handles identically. Affected files: navBar/content.js, splash/splash.js, framework/fine-print.js, framework/footer-descriptions.js, info/byline.js. Dynamic extension loading (src/util/extensions.ts): The extensions.ts file retains its synchronous require(`@extensions/...`) pattern. This works because webpack's javascript/auto parser mode (set via the module rule) handles require() at build time even in a "type": "module" package. An earlier approach using top-level await with dynamic import() caused async module semantics to cascade through the dependency graph (via util/globals.js), which broke component rendering by wrapping module exports in async module namespace objects. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Convert 8 cli files from .js to .ts, leveraging Node 24's native type stripping to run them directly without a build step. tsc is used for type-checking only (noEmit). Files converted: - cli/utils.ts - cli/server/getDataset.ts - cli/server/getDatasetHelpers.ts - cli/server/getAvailable.ts - cli/server/getNarrative.ts - cli/server/processPaths.ts - cli/view.ts - cli/develop.ts - cli/build.ts Supporting changes: - Add cli/tsconfig.json extending root with nodenext module resolution and allowImportingTsExtensions for Node-native .ts imports - Add allowImportingTsExtensions to root tsconfig.json (needed because cli .ts files are pulled in transitively via test imports) - Exclude cli/**/* from root tsconfig include/add to exclude (prevents checking cli files under the wrong moduleResolution) - Update package.json type-check script to run both tsconfigs - Update all import paths across 11 files (.js -> .ts extensions) - Update webpack.config.cjs require for cli/utils.ts - Add eslint-disable for explicit-function-return-type and consistent-type-assertions in converted files (incremental migration) - Apply chalk 2.x workaround (type assertion) for nodenext compat - Fix TypeScript strict errors: unknown in catch blocks, Object.entries casting for Set methods, undefined-in-object check Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The docs and code had fallen out of sync over the years.
* `parseNarrativeFile` was not only deprecated as the docs indicated
("This function is deprecated as of vXXX"), it no longer existed in the codebase.
* The docs for the getNarrative route incorrectly indicated the `type` query as optional
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Triggered by existing warnings: WARNING: You are currently running a version of TypeScript which is not officially supported by @typescript-eslint/typescript-estree.
Errors were new since version bump in parent commit
This is an old and now unused test that's been fully superseded by the Playwright smoke tests. It was the only test that used "chai" (and "mocha", although that wasn't listed as a dep). I only noticed this as the eslint version bump flagged up an error in the test file
This functionality never worked as well as hoped. Bigger picture, this represents a move away from pre-provisioned datasets / narratives and towards "bring your own data", which is the main use case after all!
(This was from an ancient method of deploying auspice)
to be compatible in a world without the main 'data/' directory. This made me realise our "smoke-tests" are basically a single test! Nonetheless it's worth keeping around the architecture for future improvements
This is a relic of earlier times where it was kept in-sync with the canonical nextstrain datasets. We maintain similar functionality in the fetch-test-data script but for testing purposes only. This is a key part of the bigger re-design which moves auspice to a visualisation program/library which you point at your own data.
It has previously been useful to run Auspice as a stand-alone Heroku (review-) app, but this has been superseded by standing up a testing nextstrain.org review app as the range of datasets/narratives available is much greater, or a auspice.us app for datasets not available via nextstrain.org. It's simpler (and cheaper!) to drop the Auspice app entirely
Slightly out of date!!!
[v3] [cli] Allow one or more positional path args
[v3] Various JS/CS CJS/ESM updates
[v3] Default data changes
A tip-radius update (dispatched on legend-item mouseenter, and on map deme hover) that landed while a tree position transition was still running — e.g. zooming a clade shortly after hovering a division in the legend — used a zero-duration d3 transition. Starting that transition interrupted the in-flight cx/cy transition on the very same tips, freezing them at their pre-zoom positions while the rest of the tree zoomed. On /ebola this left every tip of the hovered division (e.g. "Western Urban") stranded. genericSelectAndModify now applies transitionTime===0 updates directly instead of through a zero-duration transition, so they update the attrs/styles immediately without cancelling other running transitions on those elements. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Member
|
This is essentially a reversion of 5a66b77, which is itself a partial reversion of 3a18d77 So these changes fix the bug described here (confirmed for me too), they bring back bugs described in #1904 -- for instance see here how changing the browser dimensions doesn't trigger the tree to correctly reposition. I'll try and debug some more and see if we can get a better solution... Untitled.mov |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I discovered this bug randomly. To reproduce, go to nextstrain.org/ebola/ebov-2013, then without doing anything else, click on branch NODE_0001155 highlighted here:
After the zoom transition you'll see that all the "Western Urban" tips haven't moved.
Eventually we tracked this down to the fact that after clicking on the branch, the legend briefly opens, the cursor is over the legend swatch for "Western Urban" and this triggers a separate transition to resize Western Urban tip radii. This cancels the original transition and the Western Urban tips are left in place. The below fix seems to work and should catch other edge cases.
Below via Claude
Summary
Zooming into a clade could leave a subset of tips frozen at their pre-zoom positions while the rest of the tree zoomed. Reproduced reliably on
/ebola: hover a division in the legend (e.g. Western Urban), then click a branch to zoom — every tip of that division stays put.Root cause
A zoom triggers two
change()calls in quick succession:changeVisibility+zoom) — a 500ms d3 transition animating every tip'scx/cyto its new position.onMouseEnter(updateTipRadii({selectedLegendItem}),src/components/tree/legend/item.js). Landing <1s after the zoom, itstransitionTimeis0, so it started a zero-duration transition on exactly the tips whose radius changed.In d3, starting a transition interrupts any running (default-named) transition on the same elements. So the zero-duration radius transition cancelled those tips' in-flight
cx/cyanimation, and the subsequenttimerFlush()completed everyone else — leaving the hovered division's tips stranded.This also explains the reported quirks: it only reproduced on fresh load → zoom (the hovered
selectedLegendItempersisted; opening/closing the legend cleared it), and it was keyed on the legend value rather than tree structure. The node data and bindings were correct —mapToScreencomputed the right new positions andd.updatewas set — the DOM writes were simply cancelled mid-flight.It's a general issue: any tip-radius change (legend hover, or map deme hover) during any tree transition (zoom, filter, layout change) would freeze the matching tips.
Fix
genericSelectAndModifynow appliestransitionTime === 0updates directly (selection.call(updateCall)) rather than through a zero-duration transition, so they update the attrs/styles immediately without interrupting other running transitions on those elements.Test plan
/ebola→ zoomNODE_0001155: previously 144 tips (all Western Urban) stayed frozen; now 0 stuck, all 1493 tips reposition.dmax=2014-08-01); no console errors.tscandeslintclean.🤖 Generated with Claude Code