feat(ui): viewer chrome, edit-surface rework, figure export and touch support (the full UI/UX proposal) - #214
Merged
Merged
Conversation
timurbazhirov
changed the title
✅ Deploy Preview for mat3ra-mave ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
The proposal's position was that half-support is the worst of the three options, and the viewer was squarely in it. Verified on a 390x844 touch profile before changing anything: - The canvas had `touch-action: auto`, so the browser claimed every touch drag for page scroll and pinch-zoom and sent `pointercancel` to whatever had begun tracking it. Nothing draggable worked with a finger - not orbiting, not moving an atom, not marquee select - even though every handler in the editor is already pointer-event based. A one-finger swipe left the rendered frame byte-identical. - The quick-toggle row, the primary control surface on a phone, was 32 px. - The only surface documenting how to drive the viewer opened on `?` alone. - `isMobile` was computed from the viewport width and forwarded to exactly one dropdown. That is also the wrong question twice over: a narrow window on a desktop is not touch input, and a touch laptop is touch input at any width. Changes: - `touch-action: none` on the renderer canvas. One declaration, and every existing pointer handler starts working with a finger. - Edit mode reserves the first finger, mirroring what it already does with the left mouse button (D-4): while editing, `touches.ONE` is taken off the camera and orbit moves to two fingers (DOLLY_ROTATE). There is no right button to move it to. Restored on exit, verified across repeated round trips. - `utils/inputCapabilities.ts` replaces the viewport guess with capability queries read at call time - a window can move to another display and a tablet can gain a keyboard mid-session. `hasCoarsePointer` gates sizing; `hasTouchSupport` gates documentation, because a touch laptop needs the gestures listed and should keep its compact chrome. - Quick toggles grow to 44 px under `(pointer: coarse)` only. - The shortcut sheet is reachable from View > Shortcuts & gestures, gains a visible Close (the previous exits were `?`, Escape, or knowing that tapping the backdrop works - none available or discoverable on a phone), lists the touch gestures, and puts them first when the pointer is coarse: the sheet reflows to one column there, so group order is scroll distance, and the gestures were below three groups of keyboard shortcuts. - Nothing names an input the device lacks. The edit pill reads "drag atom = move - 2 fingers = orbit" on touch instead of RMB/Del/Esc; the sheet is titled "Shortcuts & gestures" rather than "Keyboard shortcuts" and drops the "? or Esc to close" hint on a coarse pointer. Two of these came only from looking at the rendered result: the sheet still promised `?` and Escape on a device with neither and had no visible way out, and the touch group was last. Re-verified on the same touch profile: main canvas touch-action none, every button at least 44x44, one-finger drag rotates, the sheet opens from the menu with touch first and lists pinch, pill wording correct, no console errors. Desktop rendering and wording unchanged. 32 new tests (440 passing, was 410).
Both P2 items were held on "where does wave.js stop and the host app begin".
Each answered it once the code was in front of it, which is the note worth
keeping: figure export needs the scene graph, camera frustum and renderer clear
state, none of which a host can reach, so it lives here; touch support turned out
not to be a mobile port at all - the layout already fits at 390 px, measured -
so the question dissolved into "stop half-doing it".
Also records the pre-existing bugs verification surfaced (the orthographic
projection matrix never updated after the frustum was fitted; touch-action auto
swallowing every touch drag; the dead style.width pair in initRenderer, left
alone deliberately), and the four new not-a-defect findings, so none of them are
re-investigated: the full-width lines in exported figures are the axes overlay
and are faithful to the screen; querySelector("canvas") returns the 100x100 axes
indicator, not the viewer; and two CI runs per commit is the chain being pushed
under two branch names, not a race.
timurbazhirov
changed the title
`npm run lint` - what CI runs - passes --report-unused-disable-directives, which `npx eslint src tests` does not. My local check used the latter, so a `class-methods-use-this` disable that the rule never fired for read as clean locally and failed the verify job. Rather than delete just the comment: `isChromeLineColor` takes a colour and returns a boolean about it, using no instance state, so it belongs at module scope. Moving it there removes the reason the directive existed.
…text The filter that hides edit-mode gestures from a read-only viewer matched against a Set of label strings, so rewording a label would have silently stopped hiding it. The mouse gestures express the same thing structurally, through group: "edit"; the touch rows are grouped by input device instead, so they now carry an editOnly flag of their own.
…ded layout Same change as ae9cbaa on the figure-export branch, applied here so the PR under review carries it. ModePill is merged rather than replaced: it keeps U-13's coarse-pointer wording and gains the container-width behaviour. From review of the deploy preview, all three reproduced by measuring the live DOM before changing anything: - Atomic radius stopped at 10x. It multiplies each element's van der Waals radius, so 1 is already space-filling; the top 90% of the range was unusable and left the useful band around the 0.2 default as a few pixels of travel. Now 0.1-1. - Parameters said too much: a two-line paragraph under every control, trimmed to one line each keeping the range and what the number multiplies. The per-cell count left the cost line because the status bar already shows it. - View toggles were janky: the keycap slot was only rendered on rows that had a hotkey, so the switches landed at two x positions 32 px apart. The slot is now always present, empty and hidden at a fixed width; all nine measure at one x. - The embedded case was a gap in my own verification - I checked a wide-but-short viewport and never a narrow one, which is what a host panel actually gives. At 520x900 the pill was 520 px and overlapped the icon strip, the inspector and the edit toolbar at once; at 700 and 900 it still overlapped the inspector. The container's insets now describe the space that is genuinely free, and the pill observes that space, dropping its bindings below 460 px and moving left. A media query would be the wrong instrument - an embedded panel is narrow inside a wide window, the same mistake as the viewport isMobile U-13 removes.
… size Reported from review: "Gif recording now dependent on the size of the window, must be restricted by the square canvas like we had before." createRotatingGifData read gifWidth/gifHeight straight off the canvas, so every GIF came out the shape of whoever's window recorded it - a 1400x620 window gave a 1400x620 GIF, letterboxed wherever it was embedded. A rotating structure also wants a square specifically: it sweeps through its own width as it turns, so a frame narrower than the structure's largest dimension clips at the extremes. Frames are now captured at 512x512, overridable via options.size and clamped to what the GL context can render. 512 is smaller in area than the window-sized frames it replaces on a typical desktop, so encoding gets cheaper too. The size handling is shared with figure export rather than duplicated: getFigureImage's save/restore is extracted to beginFixedRenderSize, which returns a restore callback so a synchronous caller and an await loop over frames can each wrap it in their own try/finally. Restoring inside finally means a failure during capture or encoding cannot leave the viewer stuck at 512x512. Verified in Chromium from a 1400x620 window: the downloaded GIF's logical screen size is 512x512, atoms render round rather than stretched, the structure stays fully in frame through the rotation, and the viewer's own canvas is back to 1400x620 afterwards.
Member
Author
|
Split into a stacked pair, as requested — this PR now overlaps them, so pick one route, not both:
The two contain the same code as this PR; the only difference is that the design-doc update is split so #216 alone never claims U-13 shipped. Keeping this PR instead is equally viable — it is the same work as one squashed PR and carries the review history. What is not viable is leaving all three open. What the pair already supersedesChecked by ancestry rather than by assumption:
#216 is 50 commits ahead of Also fixed since the last review round
Generated by Claude Code |
This was referenced Aug 13, 2026
Same carry-over as 819cc24 on the figure-export branch. #213 turned out not to be fully redundant when consolidating the July-12 chain: its baselines are identical to the ones regenerated here, but three files were not, and its versions are the correct ones - AGENTS.md described a fixture-parse failure mode that no longer exists (it is the visual baselines that are LFS-tracked, and their absence fails 17 visual tests while every other suite passes), the workplan still flagged the baselines as outstanding after 5882636 regenerated them, and the snapshots .gitignore was missing the *.save.png rule that keeps move-actual-expected.sh's rollback copies out of `git add .../expected`.
This was referenced Aug 13, 2026
…ed parameters Five findings from a maintainer read of the two open branches. 1. The GIF fix put the square drawing buffer back in a `finally`, which meant it stayed square through gifshot's encode - seconds for 60 frames, all of them with the on-screen canvas stretching a 512x512 buffer across a wide viewer. The frames are already captured by then, so the restore belongs right after the capture loop; the `finally` keeps a null-guarded copy for the throwing path. 2. `useObservedWidth` declared its record outside the `[]`-dep callback, so one module-level object was shared by every mounted observer. 3. `atomRadiiScale` and the repetitions were clamped on the way out of the menu but not on the way in. A saved URL or a host's initialViewSettings could carry 3.0, which rendered at 3.0 while the slider pinned at 1.0 and then snapped there on first touch. `clampParameterSettings` puts the clamp next to the ranges that define it, and the editor runs its initial settings through it. 4. `getMaxFigureDimension()` issues three `gl.getParameter` calls, and the render path called it unconditionally - a pipeline stall per frame to compute a bound only the export dialog reads. Gated on the dialog. 5. ModePill imported a width constant from SelectionInspector, so the overlay depended on a sibling for its geometry. The shared numbers now live in `chromeLayout.ts`, which both read from.
…e API #202 (SOF-7926) landed on dev while this was open, removing the `Material.Basis` and `Material.Lattice` accessors in favour of `getBasis()` / `getLattice()` and `setLattice()`, and bumping code/esse/made to 2026.8.13-0. That commit adapted the four call sites that existed on dev. This branch carries the interactive-editor stack, which dev has never seen, so it had eleven more — in `ThreeDEditor`, `interactive_structure_editor` and `StatusBar` — plus ten in tests. All migrated. `Made.Basis` / `Made.Lattice` are static references on the namespace and are deliberately untouched. `StatusBar`'s `MaterialLike` gains `getLattice?: () => LatticeLike` in place of the `Lattice` property. Its fixtures are deliberately plain objects rather than real Materials, so they now expose the accessor as a function; a test was added for a material carrying no accessor at all, which must return "" rather than throw. Two conflicts resolved in this branch's favour, both against changes #202 made to code the editor stack had already deleted: - `materialsToThreeDSceneData` in `src/utils.js`. Removed in `7beb8ae` because it built an entire WebGL `Wave` just to serialize scene JSON, was never exported from `exports.js`, had no caller, and was the only edge in the utils <-> wave import cycle. #202 only adapted its accessors; the deletion stands. - `onThreejsEditorModalHide` in `ThreeDEditor`. Dead since the editor moved in-viewer. Verified: lint 0 errors, `tsc --noEmit` clean, 461 tests passing across 32 suites, production build clean.
timurbazhirov
added a commit
that referenced
this pull request
Aug 14, 2026
feat(ui): figure export, GIF sizing and review fixes (U-12) The UI/UX proposal (U-1..U-12): viewer chrome, edit-surface rework, and publication-quality figure export. U-13 (touch support) follows in #214.
#216 landed the same work minus U-13, so its cherry-picked commits arrive back through dev as duplicates. Content-identical changes merged cleanly; the three conflicts are the files where the two lines legitimately differ: - `ModePill.tsx` — comment rewrapping only. This branch's wording names the inspector alongside the toolbar, which is what `chromeLayout.ts` made true. The coarse-pointer bindings are U-13's and are untouched by the merge. - The design doc and the context record — #216 carries the U-12-only telling ("U-13 lands separately"); this branch carries the complete one. The complete one wins, since this is the merge that makes it true. Suite counts in both documents refreshed to the landed figures: 461 passing across 32 suites, up from the 440/30 recorded before the review pass added its tests. Verified: lint 0 errors, `tsc --noEmit` clean, 461 tests passing, build clean.
Closed
timurbazhirov
pushed a commit
that referenced
this pull request
Aug 14, 2026
…r bump Rebased onto `dev` now that #216 and #214 have landed. The earlier version of this branch targeted #216's branch and pinned versions that have since been overtaken by their own advisories — `tar-fs@^3.0.9`, `form-data@^4.0.5` and `vite@^6.4.2` are all inside the current ranges. Everything below is re-derived against `dev`'s actual installed tree. Baseline on `dev`: 61 advisories (5 critical, 25 high). Of those, 29 packages had a fix reachable without a semver-major bump; the rest need jest 27 -> 30, `gl`, `looks-same` (which also replaces the `sharp` line) or `@mat3ra/*` majors, which is separate work with real breakage risk. Result: **61 -> 36** advisories. Critical 5 -> 4, high 25 -> 10, moderate 13 -> 5. Twenty-five packages resolved, none newly flagged. ## How each version was chosen Not by hand. For every advisory npm reported as fixable, the lowest release was taken that is (a) outside the advisory range and (b) still inside the major that every installed copy already sits in — so no dependent is forced across a major it did not declare. Where a package is installed at two majors at once, each gets its own entry: `form-data` 3.0.5 *and* 4.0.6, `js-yaml` 3.15.1 *and* 4.3.1, `tar-fs` 2.1.5 *and* 3.1.3, `ws` 7.5.13 *and* 8.21.3, plus `brace-expansion`, `minimatch` and `picomatch`. That scoping is the whole point. A blanket `js-yaml: ^4` moves `eslint`, `@eslint/eslintrc` and `@istanbuljs/load-nyc-config` — all of which ask for `^3.13.1` — onto the major that removed `safeLoad`. A blanket `tar-fs: ^3` moves `prebuild-install`, which is what fetches sharp's prebuilt binary and is the exact step whose flakiness `19527bd` retries around. Three entries are deliberately absent: - **esbuild** needs none. `vite@6.4.3` declares `esbuild ^0.25.0` itself, so bumping vite carries it — which matters, because forcing `^0.25.0` onto vite 6.0.7's declared `^0.24.2` would cross a 0.x boundary npm treats as breaking. - **ip-address** is reached the same way: `socks@2.8.7` declares `ip-address ^10.0.1`, and the override only lifts it past the advisory floor. - **vite** itself is the one direct dependency here, so it moves in `devDependencies` rather than through an override. `yaml` needed its declared specs as selectors (`yaml@^1.10.0`, `yaml@^1.10.2`) rather than a bare `yaml@^1`, which npm did not match. It is scoped so vite's own `yaml ^2.4.2` is untouched. ## What is left, and why - `dompurify` (and the `@toast-ui/editor` / `@toast-ui/react-editor` pair it drags) — the advisory covers every 2.x, and `@toast-ui/editor` declares `^2.3.3`, so this is a major bump inside `@mat3ra/cove`'s tree, not ours. - `@jest/core` / `jest-cli` — jest 27 to 28+. - The `gl`, `looks-same`, `vite-plugin-node-polyfills` and `@mat3ra/*` chains, all semver-major. ## Verification The lockfile was updated **incrementally** rather than regenerated. A clean regeneration on this linux-x64 container silently dropped every non-linux optional binary — 25 `@esbuild/*`, 24 `@rollup/rollup-*` and `fsevents` — which would have broken macOS and Windows installs. All 26 esbuild and 25 rollup platform entries are present and bumped in step. Sixty collateral lockfile changes, all accounted for: the platform binaries above, two rollup targets that upstream *renamed* (`loongarch64` -> `loong64`, `powerpc64le` -> `ppc64`), `jsbn` dropped by ip-address 10, and `regenerator-runtime` no longer needed by `@babel/runtime` 7.29. Run against the real installed tree from a clean `npm ci`, not just the lockfile: - `npm run lint` 0 errors, `tsc --noEmit` clean - `npm run build` clean; bundle 5,479 kB -> 5,532 kB (+1%, newer esbuild and rollup codegen), and the built bundle parses and still carries its feature markers - **461 passing / 32 suites / 0 failing** — the same count as the branch point. The visual-regression suites are inside that number, so a renderer-affecting change would have surfaced as a pixel diff.
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.
The editor spec settled what happens when you click. This is the other half: knowing what will happen before you click, and seeing what happened after.
Design doc and mockups are in the first commit (
docs/design/uiux-improvements-2026-08.md); §0.1 there records what shipped, the measurements behind it, and every place the implementation diverged from the plan.Try it: https://deploy-preview-214--mat3ra-mave.netlify.app/ — a Netlify config is included, so every future PR gets its own preview. That is the only way to exercise what the Jest suite structurally cannot reach: pointer capture, real event ordering, CSS layout at a given viewport, GPU rendering.
The problem
Each finding was verified against the source, not inferred:
Delete, and undo itself.getCheckmarkdrew every inactive View item as a grey checkmark, so one shape carried both answers, separated only by colour.LoadingIndicator,AlertDialogandModalDialoghad zero call sites, so a failed render was an unexplained blank canvas.toDataURLon the on-screen canvas — the dark theme at whatever size the container happened to have. A screenshot, not a figure.touch-action: auto, so the browser claimed every touch drag and sentpointercancelto whatever had begun tracking it. Nothing draggable worked with a finger, despite every editor handler already being pointer-event based. Verified on a 390×844 touch profile: a one-finger swipe left the rendered frame byte-identical.What's here
667b4e6StatusBar— formula, atom count, lattice, units; composition chips fromsettings.elementColorsdoubling as legend and select-all-of-element; the package's firstaria-liveregion6591930ModePill— names the active mode and the bindings that exist in no UI, with one-click exit; populates the measurement readout10cb287KeyboardSheeton?, generated from settings; the four hard-coded editor keys moved intosettings.editorKeysConfig0bc1f915d7ddc4ToggleIndicator— switch plus keycap chip at all twelve call sites; two false claims the new chrome was making, found by driving the real appeef4704ViewerErrorBoundary+ViewerStatus— loading, empty and error states with the reason and a Retryf26ef8dEditToolbar+SelectionInspector— the split that fixes F1, bounded so neither surface can reach the status bar16a986eonEditCommit's existing{source}f628e8fsetCameraAlongCellVector— view down a, b, c or [111]da8f03d:focus-visiblering; focus was measurably invisible before319447cQuickToggles— the five repeatedly-flipped View items, one click instead of four7972622e4fb81652539cf58826367e3541f) invalidated — this is what turns CI green685038319527bdnpm ci, so an upstreamsharp/libvips 503 costs a retry instead of a red buildEach commit is self-contained and explains its own reasoning; reviewing commit-by-commit will be far easier than the combined diff. Intended to squash-land.
Notable design calls
getSettings()throughupdateState; two facts were added to that payload (selectedAtomsCount,atomsPerMeasurement) rather than adding a channel. Arity is declared on the managers, so the UI never hardcodes "a distance needs two atoms" twice.matchesEditorKeyis deliberately stricter than the inline comparisons it replaces:Ctrl+Deleteno longer removes an atom, whileShift+Deletestill does.Ctrl/Cmd+Zsafely. It's gated on having something to undo instead, so with an empty stack the event is left alone and an embedding host keeps the key for its own undo. That was the design doc's open question 4, answered without needing the host to change.WebGLRenderermeans a second GL context and a duplicate of every texture and geometry. The cost of doing it in one context is the discipline of restoring everything in afinally— size, pixel ratio, clear colour and alpha, scene background, fog, and every material colour touched — which is what the tests pin. The one-click Screenshot stays: capturing what's on screen shouldn't pay for a dialog.isMobileit replaces asked the wrong question in both directions — a narrow desktop window is not touch input, and a touch laptop is touch input at full width. Sizing keys off(pointer: coarse); gesture documentation keys off whether touch exists at all, so a touch laptop gets the gestures listed and keeps its compact chrome.RMB = orbitonly once orbit is actually enabled (it starts disabled), and says2 fingers = orbiton a touch device, which has no right button.?went unmentioned until the sheet existed. The sheet drops its "? or Esc to close" hint where neither key exists, and gained a visible Close. U-8 ships no bond count because bonds are computed asynchronously and printing an uncomputed number is the same false claim this work removes. U-12's scale bar is exact only under the orthographic camera, and the dialog says so rather than presenting a perspective approximation as a measurement.this.WaveComponentnull, and most menu handlers dereference it unguarded — so a click afterwards would throw from an event handler no boundary can catch. Rather than hardening twenty call sites, all chrome that drives the wave instance is gated on oneisViewerUsableflag, reducing the live surface to the power toggle and Retry.Two pre-existing bugs, fixed here because each was load-bearing
setOrthographicCameraFrustumnever calledupdateProjectionMatrix, so from construction until the first resize the orthographic camera projected the initial ±10 frustum frominitCamerasrather than the cell-fitted one. Invisible in a browser, whereResizeObserverfires immediately andhandleResizerepairs it — and fatal for a scale bar that reads those frustum fields. Caught by cross-checking the reported scale against the camera's own projection over a known 2 Å separation, the kind of assertion that catches a formula wrong by a constant factor when nothing else would.touch-action: autoon the canvas. One declaration, and every existing pointer handler starts working with a finger.Verification
npx tsc --noEmitclean;npx eslint src tests0 errors.7e3541f) made atom radii per-element, which legitimately changes every rendered image, and the baselines still encoded the pre-fix rendering;ab4b022had recorded the regeneration as outstanding. Verified legitimate three ways before promoting anything — diffs show thin crescents on each sphere's rim and nothing else, differences are 0.00–0.50% of pixels, and reintroducing the uniform-radius bug makes the old baselines pass again. Regenerated rather than given a pixel tolerance: any tolerance wide enough to absorb this would also absorb the radius regression these baselines exist to catch.plan/context/2026-08-12T20-25Z-uiux-implementation.md§6. Among them: MUI'sInputBase75 pxmin-widthoverriding its grid column (andMuiClassNameSetuprenaming MUI classes towave-Mui*, so a.MuiInputBase-rootselector never matches), a scale-bar label at ~5 pt at 300 dpi, and a shortcut sheet with no visible way out on a phone.touch-action: none, every button ≥ 44×44, one-finger drag rotates the camera, the sheet opens from the View menu without a keyboard with the touch group first, pill wording correct, no console errors. Desktop rendering and wording unchanged.Scope question, answered
The design doc held P2 on "where does wave.js stop and the host app begin?". Each item answered it once the code was in front of it:
Also confirmed not a defect, so it isn't re-investigated: the View menu reporting Rotate/Zoom as off at startup is correct — orbit genuinely is off until toggled (
initOrbitControls(enabled = false), confirmed by dragging the live app). The old grey checkmark said the same thing; the switch just makes it legible enough to notice.Known cost
This branch sits on the 25-commit interactive-editor stack, which is not on
dev(interactive_structure_editor.tsdoes not exist there), so the diff carries both. If that stack lands separately first, this shrinks to just the UI work.dist/being tracked accounts for a large share of the remaining line count.🤖 Generated with Claude Code
https://claude.ai/code/session_0185JNEgEJLfZZEx7jKHgwu5