From 52de775b823d5126d15f8e205e8ea525f44d3a9f Mon Sep 17 00:00:00 2001 From: PiTrem Date: Tue, 25 Aug 2026 11:39:11 +0200 Subject: [PATCH 1/6] fix(lcms): expose stable DOM hooks and stop the LC/MS stack collapsing The LC/MS layout is the only place the editor mounts three chart containers stacked in one pane (.d3Line for UV/VIS, .d3Multi for the TIC, .d3Rect for the m/z scan); every other layout mounts exactly one. A host stylesheet that stretches a single chart with `.d3Line { height: 100% }` therefore makes each of the three as tall as the whole pane, and a bounded, `overflow: hidden` host container clips everything after the first - the editor renders the UV/VIS chromatogram and silently drops the TIC and m/z graphs. Make the stack root a flex column with `min-height: 0`, so that same declaration becomes a flex-basis the three panes negotiate down to a third each and the stack fits either way. The panes were already drawn for this: H is `window.innerHeight * 0.9 * 0.8 / 3` and the svg keeps its aspect ratio, so forcing `height: 100%` on a mount only letterboxes it. The toolbar had the same shape of problem: its outlined selects are compressed to `selectInput.height = 30`, well under MUI's outlined geometry, so a shrunk InputLabel ("Submit", "Write Peaks", "Write Intensity", "Decimal") floats to about -9px, outside its own box. That rendered only as long as nothing above clipped. Reserve the room with a top padding on the toolbar card so it holds for any host. Neither is fully fixable from here, because withStyles emits opaque class names (`jss8 jss4`) that a host cannot target. Add LIST_HOST_HOOK_CLASS - `rse-cmd-bar`, `lcms-stack`, `lcms-graph-panel` - as stable hooks alongside them, document them in the host-integration contracts, and pin the literal strings in a test so a rename has to be deliberate and host-coordinated. refs: ComPlat/chemotion_ELN#3477 --- dist/components/cmd_bar/common.js | 7 ++ dist/components/cmd_bar/index.js | 5 +- dist/components/d3_line_rect/index.js | 22 ++++- dist/constants/list_graph.js | 18 +++- docs/architecture/frontend-architecture.md | 20 +++++ .../units/components/host_dom_hooks.test.js | 86 +++++++++++++++++++ src/components/cmd_bar/common.js | 7 ++ src/components/cmd_bar/index.js | 5 +- src/components/d3_line_rect/index.js | 22 ++++- src/constants/list_graph.js | 18 +++- 10 files changed, 197 insertions(+), 13 deletions(-) create mode 100644 src/__tests__/units/components/host_dom_hooks.test.js diff --git a/dist/components/cmd_bar/common.js b/dist/components/cmd_bar/common.js index b851c116..1359642d 100644 --- a/dist/components/cmd_bar/common.js +++ b/dist/components/cmd_bar/common.js @@ -42,6 +42,13 @@ MuButton.displayName = 'MuButton'; const commonStyle = exports.commonStyle = { card: { margin: '0 0 5px 52px', + // The outlined selects below (Submit, Write Peaks, Write Intensity, Decimal) are + // compressed to `selectInput.height = 30`, well under MUI's outlined geometry, so + // their shrunk InputLabel floats to about -9px - outside this box. Without the + // padding it only rendered because nothing above clipped; a host that bounds the + // editor with `overflow: hidden` truncates the labels. Reserve the room here so it + // holds for any host. + paddingTop: 10, border: '1px solid white', borderRadius: 4 }, diff --git a/dist/components/cmd_bar/index.js b/dist/components/cmd_bar/index.js index e82cf604..38578d49 100644 --- a/dist/components/cmd_bar/index.js +++ b/dist/components/cmd_bar/index.js @@ -26,6 +26,7 @@ var _r08_change_axes = _interopRequireDefault(require("./r08_change_axes")); var _r09_detector = _interopRequireDefault(require("./r09_detector")); var _r10_cv_density = _interopRequireDefault(require("./r10_cv_density")); var _format = _interopRequireDefault(require("../../helpers/format")); +var _list_graph = require("../../constants/list_graph"); var _jsxRuntime = require("react/jsx-runtime"); /* eslint-disable prefer-object-spread, function-paren-newline, react/function-component-definition, react/require-default-props */ @@ -95,7 +96,7 @@ const CmdBar = ({ }); if (prependLcMsToolbar) { return /*#__PURE__*/(0, _jsxRuntime.jsxs)("div", { - className: `${classes.card} ${classes.cardFlex}`, + className: `${_list_graph.LIST_HOST_HOOK_CLASS.CMD_BAR} ${classes.card} ${classes.cardFlex}`, children: [/*#__PURE__*/(0, _jsxRuntime.jsx)("div", { className: classes.lcMsToolbarLeft, children: prependLcMsToolbar @@ -109,7 +110,7 @@ const CmdBar = ({ }); } return /*#__PURE__*/(0, _jsxRuntime.jsxs)("div", { - className: classes.card, + className: `${_list_graph.LIST_HOST_HOOK_CLASS.CMD_BAR} ${classes.card}`, children: [hideMainEditTools ? null : /*#__PURE__*/(0, _jsxRuntime.jsxs)(_jsxRuntime.Fragment, { children: [/*#__PURE__*/(0, _jsxRuntime.jsx)(_viewer.default, { editorOnly: editorOnly diff --git a/dist/components/d3_line_rect/index.js b/dist/components/d3_line_rect/index.js index 8e598035..7537ec07 100644 --- a/dist/components/d3_line_rect/index.js +++ b/dist/components/d3_line_rect/index.js @@ -77,9 +77,20 @@ const isLcmsMsPageLoading = (mzEntities = [], hplcMsSt = {}) => { exports.isLcmsMsPageLoading = isLcmsMsPageLoading; const styles = () => Object.assign({}, { lcMsStackRoot: { - margin: '0 0 5px 52px' + margin: '0 0 5px 52px', + // This is the only place the editor mounts three chart containers stacked in one + // pane instead of a single one. A host stylesheet that stretches a single chart with + // `.d3Line { height: 100% }` would otherwise make each of the three as tall as + // this whole pane and push the TIC and m/z graphs out of a bounded, clipped + // container. As a flex column the same declaration becomes a flex-basis the + // three panes negotiate down to a third each, so the stack fits either way. + display: 'flex', + flexDirection: 'column', + minHeight: 0 }, lcMsToolbarRow: { + // Never absorb the shrink the chart panes above negotiate. + flexShrink: 0, display: 'flex', flexWrap: 'wrap', alignItems: 'center', @@ -106,7 +117,10 @@ const styles = () => Object.assign({}, { flex: '0 1 auto' }, lcMsGraphPanel: { - position: 'relative' + position: 'relative', + // Wraps the m/z chart, so it must flex like the two bare chart mounts beside it. + flex: '1 1 auto', + minHeight: 0 }, lcMsLoadingOverlay: { position: 'absolute', @@ -667,7 +681,7 @@ class ViewerLineRect extends _react.default.Component { selectWavelengthAct(event); }; return /*#__PURE__*/(0, _jsxRuntime.jsxs)("div", { - className: classes.lcMsStackRoot, + className: `${_list_graph.LIST_HOST_HOOK_CLASS.LCMS_STACK} ${classes.lcMsStackRoot}`, children: [omitUvvisToolbarRow ? null : /*#__PURE__*/(0, _jsxRuntime.jsxs)("div", { className: classes.lcMsToolbarRow, children: [/*#__PURE__*/(0, _jsxRuntime.jsxs)("div", { @@ -752,7 +766,7 @@ class ViewerLineRect extends _react.default.Component { }) })] }), /*#__PURE__*/(0, _jsxRuntime.jsxs)("div", { - className: classes.lcMsGraphPanel, + className: `${_list_graph.LIST_HOST_HOOK_CLASS.LCMS_GRAPH_PANEL} ${classes.lcMsGraphPanel}`, children: [/*#__PURE__*/(0, _jsxRuntime.jsx)("div", { className: _list_graph.LIST_ROOT_SVG_GRAPH.RECT }), isMsLoading ? /*#__PURE__*/(0, _jsxRuntime.jsx)("div", { diff --git a/dist/constants/list_graph.js b/dist/constants/list_graph.js index 24d49820..201fe622 100644 --- a/dist/constants/list_graph.js +++ b/dist/constants/list_graph.js @@ -3,7 +3,7 @@ Object.defineProperty(exports, "__esModule", { value: true }); -exports.LIST_ROOT_SVG_GRAPH = exports.LIST_BRUSH_SVG_GRAPH = void 0; +exports.LIST_ROOT_SVG_GRAPH = exports.LIST_HOST_HOOK_CLASS = exports.LIST_BRUSH_SVG_GRAPH = void 0; const LIST_ROOT_SVG_GRAPH = exports.LIST_ROOT_SVG_GRAPH = { LINE: 'd3Line', RECT: 'd3Rect', @@ -13,4 +13,20 @@ const LIST_BRUSH_SVG_GRAPH = exports.LIST_BRUSH_SVG_GRAPH = { LINE: 'd3Svg', RECT: 'd3SvgRect', MULTI: 'd3SvgMulti' +}; + +// Stable, non-JSS class names a host application (chemotion_ELN) styles against. The +// classes withStyles generates around these nodes are opaque (`jss8 jss4`) and change +// between builds, so a host stylesheet has nothing else to target. Treat these as part +// of the public DOM contract: do not rename them without a host-side change. +// +// Why hosts need them: LCMS_STACK marks the one place the editor mounts three sibling +// chart containers (`.d3Line` + `.d3Multi` + `.d3Rect`) rather than a single one, so a +// blanket `.d3Line { height: 100% }` rule triples the stack's height; CMD_BAR marks the +// toolbar row, whose outlined select labels float above their own box and must not be +// clipped by a host `overflow: hidden`. +const LIST_HOST_HOOK_CLASS = exports.LIST_HOST_HOOK_CLASS = { + CMD_BAR: 'rse-cmd-bar', + LCMS_STACK: 'lcms-stack', + LCMS_GRAPH_PANEL: 'lcms-graph-panel' }; \ No newline at end of file diff --git a/docs/architecture/frontend-architecture.md b/docs/architecture/frontend-architecture.md index e1340faa..b9318aeb 100644 --- a/docs/architecture/frontend-architecture.md +++ b/docs/architecture/frontend-architecture.md @@ -208,6 +208,26 @@ The following table maps each host contract to runtime code inside the editor. `entity` must include `layout`, `spectra`, and `features` in the shape produced by `FN.ExtractJcamp` (`{ spectra, features, layout }`, plus layout-specific fields). `GetComparisons` transforms comparison entities in `jcamp` state for IR, HPLC UV/VIS, and XRD overlays. +### DOM hooks for host stylesheets + +`withStyles` generates opaque class names (`jss8 jss4`) that change between builds, so a host +stylesheet cannot target the editor's own containers. `LIST_HOST_HOOK_CLASS` +(`src/constants/list_graph.js`) adds stable class names alongside them. They are part of the +public DOM contract — renaming one is a breaking change for the host. + +| Class | Node | Why a host needs it | +|---|---|---| +| `rse-cmd-bar` | `CmdBar` root (`src/components/cmd_bar/index.js`) | The toolbar's outlined selects are compressed to 30px, so their shrunk `InputLabel` floats above its own box. A host that bounds the editor must not clip this row. | +| `lcms-stack` | LC/MS stack root (`src/components/d3_line_rect/index.js`) | The one place the editor mounts three chart containers (`.d3Line`, `.d3Multi`, `.d3Rect`) stacked in a single pane rather than one. A blanket `.d3Line { height: 100% }` rule written for the single-chart layouts triples this stack's height. | +| `lcms-graph-panel` | m/z pane wrapper inside the stack | Wraps `.d3Rect` plus the loading overlay, so it — not `.d3Rect` — is the flex item beside the other two charts. | + +The chart mount classes themselves (`d3Line` / `d3Multi` / `d3Rect` and the inner +`d3Svg` / `d3SvgMulti` / `d3SvgRect`) are already stable (`LIST_ROOT_SVG_GRAPH`, +`LIST_BRUSH_SVG_GRAPH`). Each chart is drawn as a `viewBox` with +`preserveAspectRatio="xMinYMin meet"` (`src/components/common/draw.js`) sized from +`window.inner*` at mount, with the LC/MS height already divided by three — so forcing +`height: 100%` on a mount letterboxes the chart rather than enlarging it. + ## Runtime Synchronization Patterns Runtime synchronization is distributed across `LayerInit`, reducers, sagas, and D3 viewers. The editor relies on action propagation rather than a single central controller. diff --git a/src/__tests__/units/components/host_dom_hooks.test.js b/src/__tests__/units/components/host_dom_hooks.test.js new file mode 100644 index 00000000..bd023298 --- /dev/null +++ b/src/__tests__/units/components/host_dom_hooks.test.js @@ -0,0 +1,86 @@ +import React from 'react'; +import { render } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import { Provider } from 'react-redux'; +import { createTheme } from '@mui/material'; +import { ThemeProvider } from '@mui/styles'; + +import { store } from '../../../app'; +import CmdBar from '../../../components/cmd_bar/index'; +import ViewerLineRect from '../../../components/d3_line_rect/index'; +import { LIST_HOST_HOOK_CLASS } from '../../../constants/list_graph'; + +// chemotion_ELN styles the editor from its own stylesheet and can only target class +// names that survive a build - withStyles emits opaque `jss8 jss4` ones. These hooks are +// therefore a published contract: a rename silently breaks the host's layout (the LC/MS +// three-chart stack collapsing to its first pane, the toolbar's floating select labels +// getting clipped), with nothing failing here. Assert on the literal strings so a rename +// has to be a deliberate, host-coordinated change. +describe('host DOM hooks', () => { + it('pins the published class names', () => { + expect(LIST_HOST_HOOK_CLASS).toEqual({ + CMD_BAR: 'rse-cmd-bar', + LCMS_STACK: 'lcms-stack', + LCMS_GRAPH_PANEL: 'lcms-graph-panel', + }); + }); + + // The editor's own store, so every slice these connected components select from is + // present without hand-rolling a fixture of the whole state tree. + const theme = createTheme(); + const withStore = (ui) => render( + + {ui} + , + ); + + it('marks the CmdBar root', () => { + const { container } = withStore( + , + ); + expect(container.querySelector(`.${LIST_HOST_HOOK_CLASS.CMD_BAR}`)).toBeInTheDocument(); + }); + + // d3-tip attaches to the chart svg on mount and calls SVG geometry APIs jsdom does + // not implement. Only this suite renders a real chart, so the shims stay local. + beforeAll(() => { + const proto = window.SVGSVGElement.prototype; + proto.createSVGPoint = proto.createSVGPoint || (() => ({ + x: 0, + y: 0, + matrixTransform: () => ({ x: 0, y: 0 }), + })); + proto.getScreenCTM = proto.getScreenCTM || (() => ({ + a: 1, b: 0, c: 0, d: 1, e: 0, f: 0, inverse: () => ({ a: 1, b: 0, c: 0, d: 1, e: 0, f: 0 }), + })); + }); + + it('marks the LC/MS stack root and the m/z pane', () => { + const { container } = withStore( + , + ); + const stack = container.querySelector(`.${LIST_HOST_HOOK_CLASS.LCMS_STACK}`); + expect(stack).toBeInTheDocument(); + // The three chart mounts the host's height rules act on must all be inside the stack, + // otherwise a host rule scoped to `.lcms-stack` misses one of them. + expect(stack.querySelector('.d3Line')).toBeInTheDocument(); + expect(stack.querySelector('.d3Multi')).toBeInTheDocument(); + const panel = stack.querySelector(`.${LIST_HOST_HOOK_CLASS.LCMS_GRAPH_PANEL}`); + expect(panel).toBeInTheDocument(); + expect(panel.querySelector('.d3Rect')).toBeInTheDocument(); + }); +}); diff --git a/src/components/cmd_bar/common.js b/src/components/cmd_bar/common.js index c2819cfe..b94778cb 100644 --- a/src/components/cmd_bar/common.js +++ b/src/components/cmd_bar/common.js @@ -32,6 +32,13 @@ MuButton.displayName = 'MuButton'; const commonStyle = { card: { margin: '0 0 5px 52px', + // The outlined selects below (Submit, Write Peaks, Write Intensity, Decimal) are + // compressed to `selectInput.height = 30`, well under MUI's outlined geometry, so + // their shrunk InputLabel floats to about -9px - outside this box. Without the + // padding it only rendered because nothing above clipped; a host that bounds the + // editor with `overflow: hidden` truncates the labels. Reserve the room here so it + // holds for any host. + paddingTop: 10, border: '1px solid white', borderRadius: 4, }, diff --git a/src/components/cmd_bar/index.js b/src/components/cmd_bar/index.js index 450c109d..e8311550 100644 --- a/src/components/cmd_bar/index.js +++ b/src/components/cmd_bar/index.js @@ -23,6 +23,7 @@ import ChangeAxes from './r08_change_axes'; import Detector from './r09_detector'; import CvDensityControls from './r10_cv_density'; import Format from '../../helpers/format'; +import { LIST_HOST_HOOK_CLASS } from '../../constants/list_graph'; const styles = () => ( Object.assign( @@ -95,7 +96,7 @@ const CmdBar = ({ if (prependLcMsToolbar) { return ( -
+
{ prependLcMsToolbar }
@@ -109,7 +110,7 @@ const CmdBar = ({ } return ( -
+
{ hideMainEditTools ? null : ( <> diff --git a/src/components/d3_line_rect/index.js b/src/components/d3_line_rect/index.js index 94c562f0..c9836419 100644 --- a/src/components/d3_line_rect/index.js +++ b/src/components/d3_line_rect/index.js @@ -37,7 +37,9 @@ import { import { LIST_UI_SWEEP_TYPE, LIST_NON_BRUSH_TYPES } from '../../constants/list_ui'; import renderWavelengthSelect from '../../features/lc-ms/ui/wavelengthSelect'; import { parseFeaturePageValue as parsePageValue } from '../../features/lc-ms/parsing/pageValue'; -import { LIST_ROOT_SVG_GRAPH, LIST_BRUSH_SVG_GRAPH } from '../../constants/list_graph'; +import { + LIST_ROOT_SVG_GRAPH, LIST_BRUSH_SVG_GRAPH, LIST_HOST_HOOK_CLASS, +} from '../../constants/list_graph'; import PeakGroup from '../cmd_bar/08_peak_group'; import Threshold from '../cmd_bar/r03_threshold'; import Integration from '../cmd_bar/04_integration'; @@ -89,8 +91,19 @@ const styles = () => ( { lcMsStackRoot: { margin: '0 0 5px 52px', + // This is the only place the editor mounts three chart containers stacked in one + // pane instead of a single one. A host stylesheet that stretches a single chart with + // `.d3Line { height: 100% }` would otherwise make each of the three as tall as + // this whole pane and push the TIC and m/z graphs out of a bounded, clipped + // container. As a flex column the same declaration becomes a flex-basis the + // three panes negotiate down to a third each, so the stack fits either way. + display: 'flex', + flexDirection: 'column', + minHeight: 0, }, lcMsToolbarRow: { + // Never absorb the shrink the chart panes above negotiate. + flexShrink: 0, display: 'flex', flexWrap: 'wrap', alignItems: 'center', @@ -118,6 +131,9 @@ const styles = () => ( }, lcMsGraphPanel: { position: 'relative', + // Wraps the m/z chart, so it must flex like the two bare chart mounts beside it. + flex: '1 1 auto', + minHeight: 0, }, lcMsLoadingOverlay: { position: 'absolute', @@ -660,7 +676,7 @@ class ViewerLineRect extends React.Component { selectWavelengthAct(event); }; return ( -
+
{ omitUvvisToolbarRow ? null : (
@@ -733,7 +749,7 @@ class ViewerLineRect extends React.Component {
-
+
{ isMsLoading ? ( diff --git a/src/constants/list_graph.js b/src/constants/list_graph.js index 7ea69bb8..c291b797 100644 --- a/src/constants/list_graph.js +++ b/src/constants/list_graph.js @@ -10,6 +10,22 @@ const LIST_BRUSH_SVG_GRAPH = { MULTI: 'd3SvgMulti', }; +// Stable, non-JSS class names a host application (chemotion_ELN) styles against. The +// classes withStyles generates around these nodes are opaque (`jss8 jss4`) and change +// between builds, so a host stylesheet has nothing else to target. Treat these as part +// of the public DOM contract: do not rename them without a host-side change. +// +// Why hosts need them: LCMS_STACK marks the one place the editor mounts three sibling +// chart containers (`.d3Line` + `.d3Multi` + `.d3Rect`) rather than a single one, so a +// blanket `.d3Line { height: 100% }` rule triples the stack's height; CMD_BAR marks the +// toolbar row, whose outlined select labels float above their own box and must not be +// clipped by a host `overflow: hidden`. +const LIST_HOST_HOOK_CLASS = { + CMD_BAR: 'rse-cmd-bar', + LCMS_STACK: 'lcms-stack', + LCMS_GRAPH_PANEL: 'lcms-graph-panel', +}; + export { - LIST_ROOT_SVG_GRAPH, LIST_BRUSH_SVG_GRAPH, + LIST_ROOT_SVG_GRAPH, LIST_BRUSH_SVG_GRAPH, LIST_HOST_HOOK_CLASS, }; From 0156c75c771ab7b9a0ba80a950ff71495fd2e0c3 Mon Sep 17 00:00:00 2001 From: PiTrem Date: Tue, 25 Aug 2026 19:48:31 +0200 Subject: [PATCH 2/6] fix(panel): make the info/multiplicity panel a real scroll container MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The right-hand accordion panel (Info, Multiplicity, Peaks, …) is truncated at the bottom once its content is tall, with no way to reach the rest. `panels` declared `display: table` above `max-height` and `overflow-y: auto`, and a table box honours neither: CSS leaves the effect of `max-height` on a table undefined, and `overflow` does not make one a scroll container. Both declarations were dead. Measured in Chrome with five expanded accordions, the panel renders 1512px tall inside a 748px column - it never capped, never scrolled, and simply overflowed whatever contained it. That was invisible for as long as the host let the page grow around it. A host that bounds the editor and clips the overflow instead - as chemotion_ELN now does - shows the panel cut off at the bottom, which is the reported symptom. Make it a block box, and take the height from a bounded parent via `height: 100%` + `min-height: 0`. Against an unbounded parent `height: 100%` computes to `auto`, so the existing viewport-derived `max-height` still governs there - and now actually applies, capping the same content at 674px with a working scrollbar instead of 1512px without one. Verified in headless Chrome across both cases (bounded host column and unbounded standalone), at 1920x1080 and 1366x768. --- dist/components/panel/index.js | 14 +++++++++++++- src/components/panel/index.js | 14 +++++++++++++- 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/dist/components/panel/index.js b/dist/components/panel/index.js index 80bc732a..a5258aa7 100644 --- a/dist/components/panel/index.js +++ b/dist/components/panel/index.js @@ -34,9 +34,21 @@ const theme = (0, _styles.createTheme)({ }); const styles = () => ({ panels: { + // `display: table` silently defeated both declarations below it: CSS leaves the + // effect of `max-height` on a table box undefined, and `overflow` does not make one + // a scroll container. So this panel never capped and never scrolled - it grew to the + // full height of its accordions (measured at 1512px for five expanded panels against + // a 748px column) and simply overflowed whatever contained it. That went unnoticed + // while the host let the page grow; a host that bounds the editor and clips it + // instead shows the panel truncated at the bottom with no way to scroll to the rest. + display: 'block', + // Take the height from a bounded parent when there is one; `height: 100%` against an + // unbounded parent computes to `auto`, so the viewport-derived cap below still + // applies in a host that does not constrain us (and now actually works). + height: '100%', + minHeight: 0, maxHeight: 'calc(90vh - 220px)', // ROI - display: 'table', overflowX: 'hidden', overflowY: 'auto', margin: '5px 0 0 0', diff --git a/src/components/panel/index.js b/src/components/panel/index.js index 7461dc40..214d9afe 100644 --- a/src/components/panel/index.js +++ b/src/components/panel/index.js @@ -32,8 +32,20 @@ const theme = createTheme({ const styles = () => ({ panels: { + // `display: table` silently defeated both declarations below it: CSS leaves the + // effect of `max-height` on a table box undefined, and `overflow` does not make one + // a scroll container. So this panel never capped and never scrolled - it grew to the + // full height of its accordions (measured at 1512px for five expanded panels against + // a 748px column) and simply overflowed whatever contained it. That went unnoticed + // while the host let the page grow; a host that bounds the editor and clips it + // instead shows the panel truncated at the bottom with no way to scroll to the rest. + display: 'block', + // Take the height from a bounded parent when there is one; `height: 100%` against an + // unbounded parent computes to `auto`, so the viewport-derived cap below still + // applies in a host that does not constrain us (and now actually works). + height: '100%', + minHeight: 0, maxHeight: 'calc(90vh - 220px)', // ROI - display: 'table', overflowX: 'hidden', overflowY: 'auto', margin: '5px 0 0 0', From 99023c469d8104dbce3f842eb26817cdc5b325be Mon Sep 17 00:00:00 2001 From: PiTrem Date: Tue, 25 Aug 2026 19:53:48 +0200 Subject: [PATCH 3/6] fix(lcms): even out the three pane heights, and harden the hook contract Follow-up on this branch's own review, most of it caught by measuring the layout in a browser rather than by reading it. Pane heights. Giving the m/z wrapper `flex: 1 1 auto` while a host puts `height: 100%` on the two bare chart mounts left the three panes with different flex bases, so the shrink was shared in proportion to basis and m/z came out at half the height of its siblings (measured 130px against 258px). Restating `height: 100%` on all three from here makes the bases agree: 215/215/215. Not `flex-basis: 0`, which was the first attempt and was worse - a host stylesheet loads after this JSS and chemotion_ELN's rule for `.d3Line`/`.d3Multi` outranks anything reachable from a JSS child selector, so a basis of 0 applied to the m/z panel alone and collapsed it to zero height. Measured, not reasoned: the regression only showed up on re-running the browser probe. This also makes the stack self-contained. It previously depended on the host supplying `flex`/`min-height` to the two mounts; now a host that merely bounds the editor's height gets three even panes without shipping a rule for them, and the companion `spectra.scss` change becomes an optimisation rather than a prerequisite. Hook contract: - Prefix the new hooks (`rse-lcms-stack`, `rse-lcms-graph-panel`). They land in a host's global, non-modular stylesheet next to its own classes, and the no-rename rule makes a generic name expensive to undo later. - Add `EDITOR_ROOT` for `react-spectrum-editor` - the oldest hook and the one hosts actually target, previously a bare string in four components and absent from the contract it belongs to. Keeps its historical unprefixed name. - Export `LIST_HOST_HOOK_CLASS` from the package entry point, so a host can reference the strings instead of deep-importing dist/ or hard-coding them. Toolbar budget: `commonStyle.card` now reserves 10px, so the two layouts that size against a hard-coded constant give it back - `calc(90vh - 220px)` becomes `230px` in the CV editor and the info panel. Without that they overflow by exactly the padding. Tests: assert the hook names per entry rather than with `toEqual` on the whole object, so adding a fourth hook is not a breaking change; drop a tautological case; remove an `hplcMsSt` prop that react-redux's default mergeProps discards; fix the eslint offences in the new file and the misleading comment about `beforeAll` scope. --- dist/app.js | 7 ++ dist/components/d3_line_rect/index.js | 36 +++++++--- dist/components/hplc_viewer.js | 3 +- dist/components/multi_jcamps_viewer.js | 8 ++- dist/components/panel/index.js | 3 +- dist/constants/list_graph.js | 18 +++-- dist/layer_prism.js | 5 +- docs/architecture/frontend-architecture.md | 12 +++- .../units/components/host_dom_hooks.test.js | 70 +++++++++++-------- src/app.js | 6 ++ src/components/d3_line_rect/index.js | 32 +++++++-- src/components/hplc_viewer.js | 3 +- src/components/multi_jcamps_viewer.js | 8 ++- src/components/panel/index.js | 3 +- src/constants/list_graph.js | 18 +++-- src/layer_prism.js | 5 +- 16 files changed, 166 insertions(+), 71 deletions(-) diff --git a/dist/app.js b/dist/app.js index d66e1117..13b67274 100644 --- a/dist/app.js +++ b/dist/app.js @@ -10,6 +10,12 @@ Object.defineProperty(exports, "FN", { return _fn.default; } }); +Object.defineProperty(exports, "LIST_HOST_HOOK_CLASS", { + enumerable: true, + get: function get() { + return _list_graph.LIST_HOST_HOOK_CLASS; + } +}); exports.store = exports.SpectraEditor = void 0; var _react = _interopRequireDefault(require("react")); var _reactRedux = require("react-redux"); @@ -22,6 +28,7 @@ var _index = _interopRequireDefault(require("./reducers/index")); var _index2 = _interopRequireDefault(require("./sagas/index")); var _layer_init = _interopRequireDefault(require("./layer_init")); var _fn = _interopRequireDefault(require("./fn")); +var _list_graph = require("./constants/list_graph"); var _jsxRuntime = require("react/jsx-runtime"); /* eslint-disable react/function-component-definition, react/require-default-props */ diff --git a/dist/components/d3_line_rect/index.js b/dist/components/d3_line_rect/index.js index 7537ec07..0aa54664 100644 --- a/dist/components/d3_line_rect/index.js +++ b/dist/components/d3_line_rect/index.js @@ -79,14 +79,32 @@ const styles = () => Object.assign({}, { lcMsStackRoot: { margin: '0 0 5px 52px', // This is the only place the editor mounts three chart containers stacked in one - // pane instead of a single one. A host stylesheet that stretches a single chart with - // `.d3Line { height: 100% }` would otherwise make each of the three as tall as - // this whole pane and push the TIC and m/z graphs out of a bounded, clipped - // container. As a flex column the same declaration becomes a flex-basis the - // three panes negotiate down to a third each, so the stack fits either way. + // pane instead of a single one. A host stylesheet that stretches a single chart + // with `.d3Line { height: 100% }` would otherwise make each of the three as tall + // as this whole pane and push the TIC and m/z graphs out of a bounded, clipped + // container. As a flex column they share the pane instead, so the stack fits + // whether or not the host ships such a rule. display: 'flex', flexDirection: 'column', - minHeight: 0 + // Load-bearing under a host that makes this node a flex item (chemotion_ELN's + // `.MuiGrid-grid-xs-9 { display: flex; flex-direction: column }` does), where the + // initial `min-height: auto` would otherwise resolve to the stack's content size + // and defeat the shrink below. + minHeight: 0, + // All three panes need the SAME flex-basis or the deficit is shared in proportion + // to basis and one of them is squeezed to a fraction of a third. `height: 100%` + // rather than `flex-basis: 0`, because a host stylesheet loads after this JSS and + // chemotion_ELN already puts `height: 100%` on the two bare mounts with a higher + // specificity than anything reachable from here - a basis of 0 would lose that + // contest on `.d3Line`/`.d3Multi`, apply to the m/z panel alone, and collapse it to + // zero. Restating the same declaration agrees with such a host and supplies it for + // one that bounds our height without styling the mounts itself. Against an + // unbounded parent it computes to `auto`, i.e. the content height, as before. + '& > .d3Line, & > .d3Multi': { + flex: '1 1 auto', + minHeight: 0, + height: '100%' + } }, lcMsToolbarRow: { // Never absorb the shrink the chart panes above negotiate. @@ -118,9 +136,11 @@ const styles = () => Object.assign({}, { }, lcMsGraphPanel: { position: 'relative', - // Wraps the m/z chart, so it must flex like the two bare chart mounts beside it. + // Wraps the m/z chart plus its loading overlay, so this - not `.d3Rect` - is the + // flex item beside the two bare mounts, and takes the same basis as they do. flex: '1 1 auto', - minHeight: 0 + minHeight: 0, + height: '100%' }, lcMsLoadingOverlay: { position: 'absolute', diff --git a/dist/components/hplc_viewer.js b/dist/components/hplc_viewer.js index a23fb588..e3234e94 100644 --- a/dist/components/hplc_viewer.js +++ b/dist/components/hplc_viewer.js @@ -16,6 +16,7 @@ var _index2 = _interopRequireDefault(require("./cmd_bar/index")); var _index3 = _interopRequireDefault(require("./d3_line_rect/index")); var _lc_ms_uv_tools_bar = _interopRequireDefault(require("./lc_ms_uv_tools_bar")); var _extractEntityLCMS = require("../helpers/extractEntityLCMS"); +var _list_graph = require("../constants/list_graph"); var _jsxRuntime = require("react/jsx-runtime"); /* eslint-disable react/default-props-match-prop-types, react/require-default-props, react/no-unused-prop-types, react/jsx-boolean-value, @@ -87,7 +88,7 @@ class HPLCViewer extends _react.default.Component { hideMainEditTools: true, prependLcMsToolbar: /*#__PURE__*/(0, _jsxRuntime.jsx)(_lc_ms_uv_tools_bar.default, {}) }), /*#__PURE__*/(0, _jsxRuntime.jsx)("div", { - className: "react-spectrum-editor", + className: _list_graph.LIST_HOST_HOOK_CLASS.EDITOR_ROOT, children: /*#__PURE__*/(0, _jsxRuntime.jsxs)(_material.Grid, { container: true, children: [/*#__PURE__*/(0, _jsxRuntime.jsx)(_material.Grid, { diff --git a/dist/components/multi_jcamps_viewer.js b/dist/components/multi_jcamps_viewer.js index 0cd897f7..df3467ae 100644 --- a/dist/components/multi_jcamps_viewer.js +++ b/dist/components/multi_jcamps_viewer.js @@ -20,6 +20,7 @@ var _curve = require("../actions/curve"); var _cyclic_voltammetry = require("../actions/cyclic_voltammetry"); var _list_layout = require("../constants/list_layout"); var _format = _interopRequireDefault(require("../helpers/format")); +var _list_graph = require("../constants/list_graph"); var _jsxRuntime = require("react/jsx-runtime"); /* eslint-disable react/default-props-match-prop-types, react/require-default-props, react/no-unused-prop-types, react/jsx-boolean-value, @@ -37,7 +38,10 @@ const styles = () => ({ fontSize: '14px' }, cvEditor: { - height: 'calc(90vh - 220px)', + // 230, not 220: `commonStyle.card` reserves 10px above the toolbar for the + // outlined selects' floating labels, and this constant is the budget left + // over after it. Keep the two in step. + height: 'calc(90vh - 230px)', display: 'flex', flexDirection: 'column', minHeight: 0, @@ -130,7 +134,7 @@ class MultiJcampsViewer extends _react.default.Component { editorOnly: editorOnly, hideThreshold: !_format.default.isNmrLayout(layoutSt) }), /*#__PURE__*/(0, _jsxRuntime.jsx)("div", { - className: (0, _classnames.default)('react-spectrum-editor', isCyclicVolta && classes.cvEditor), + className: (0, _classnames.default)(_list_graph.LIST_HOST_HOOK_CLASS.EDITOR_ROOT, isCyclicVolta && classes.cvEditor), children: /*#__PURE__*/(0, _jsxRuntime.jsxs)(_Grid.default, { container: true, className: isCyclicVolta ? classes.cvTopRow : undefined, diff --git a/dist/components/panel/index.js b/dist/components/panel/index.js index a5258aa7..bf10fc9d 100644 --- a/dist/components/panel/index.js +++ b/dist/components/panel/index.js @@ -47,7 +47,8 @@ const styles = () => ({ // applies in a host that does not constrain us (and now actually works). height: '100%', minHeight: 0, - maxHeight: 'calc(90vh - 220px)', + // 230, not 220 - see the matching constant in multi_jcamps_viewer.js. + maxHeight: 'calc(90vh - 230px)', // ROI overflowX: 'hidden', overflowY: 'auto', diff --git a/dist/constants/list_graph.js b/dist/constants/list_graph.js index 201fe622..4e2e91cc 100644 --- a/dist/constants/list_graph.js +++ b/dist/constants/list_graph.js @@ -20,13 +20,17 @@ const LIST_BRUSH_SVG_GRAPH = exports.LIST_BRUSH_SVG_GRAPH = { // between builds, so a host stylesheet has nothing else to target. Treat these as part // of the public DOM contract: do not rename them without a host-side change. // -// Why hosts need them: LCMS_STACK marks the one place the editor mounts three sibling -// chart containers (`.d3Line` + `.d3Multi` + `.d3Rect`) rather than a single one, so a -// blanket `.d3Line { height: 100% }` rule triples the stack's height; CMD_BAR marks the -// toolbar row, whose outlined select labels float above their own box and must not be -// clipped by a host `overflow: hidden`. +// All entries are `rse-` prefixed. These land in a host's global, non-modular stylesheet +// alongside its own classes, so an unprefixed generic name like `lcms-stack` would be one +// collision away from a host's own LC/MS markup - and the contract above makes such a name +// expensive to change afterwards. +// +// EDITOR_ROOT is the oldest of these and the one hosts already target; it is listed here +// so it is covered by the same contract as the rest, rather than living on as a bare +// string literal in four components. const LIST_HOST_HOOK_CLASS = exports.LIST_HOST_HOOK_CLASS = { + EDITOR_ROOT: 'react-spectrum-editor', CMD_BAR: 'rse-cmd-bar', - LCMS_STACK: 'lcms-stack', - LCMS_GRAPH_PANEL: 'lcms-graph-panel' + LCMS_STACK: 'rse-lcms-stack', + LCMS_GRAPH_PANEL: 'rse-lcms-graph-panel' }; \ No newline at end of file diff --git a/dist/layer_prism.js b/dist/layer_prism.js index 07650926..30b3b6f2 100644 --- a/dist/layer_prism.js +++ b/dist/layer_prism.js @@ -16,6 +16,7 @@ var _index2 = _interopRequireDefault(require("./components/cmd_bar/index")); var _layer_content = _interopRequireDefault(require("./layer_content")); var _list_ui = require("./constants/list_ui"); var _extractParams = require("./helpers/extractParams"); +var _list_graph = require("./constants/list_graph"); var _jsxRuntime = require("react/jsx-runtime"); /* eslint-disable prefer-object-spread, default-param-last, react/function-component-definition, react/require-default-props @@ -74,7 +75,7 @@ const LayerPrism = ({ operations: operations, editorOnly: editorOnly }), /*#__PURE__*/(0, _jsxRuntime.jsx)("div", { - className: "react-spectrum-editor", + className: _list_graph.LIST_HOST_HOOK_CLASS.EDITOR_ROOT, children: /*#__PURE__*/(0, _jsxRuntime.jsx)(_Grid.default, { container: true, children: /*#__PURE__*/(0, _jsxRuntime.jsx)(_Grid.default, { @@ -103,7 +104,7 @@ const LayerPrism = ({ operations: operations, editorOnly: editorOnly }), /*#__PURE__*/(0, _jsxRuntime.jsx)("div", { - className: "react-spectrum-editor", + className: _list_graph.LIST_HOST_HOOK_CLASS.EDITOR_ROOT, children: /*#__PURE__*/(0, _jsxRuntime.jsxs)(_Grid.default, { container: true, children: [/*#__PURE__*/(0, _jsxRuntime.jsx)(_Grid.default, { diff --git a/docs/architecture/frontend-architecture.md b/docs/architecture/frontend-architecture.md index b9318aeb..2f87d15d 100644 --- a/docs/architecture/frontend-architecture.md +++ b/docs/architecture/frontend-architecture.md @@ -217,9 +217,15 @@ public DOM contract — renaming one is a breaking change for the host. | Class | Node | Why a host needs it | |---|---|---| -| `rse-cmd-bar` | `CmdBar` root (`src/components/cmd_bar/index.js`) | The toolbar's outlined selects are compressed to 30px, so their shrunk `InputLabel` floats above its own box. A host that bounds the editor must not clip this row. | -| `lcms-stack` | LC/MS stack root (`src/components/d3_line_rect/index.js`) | The one place the editor mounts three chart containers (`.d3Line`, `.d3Multi`, `.d3Rect`) stacked in a single pane rather than one. A blanket `.d3Line { height: 100% }` rule written for the single-chart layouts triples this stack's height. | -| `lcms-graph-panel` | m/z pane wrapper inside the stack | Wraps `.d3Rect` plus the loading overlay, so it — not `.d3Rect` — is the flex item beside the other two charts. | +| `react-spectrum-editor` | editor root, below `CmdBar` (`hplc_viewer.js`, `layer_prism.js`, `multi_jcamps_viewer.js`) | The node every host stylesheet already bounds and clips. Unprefixed for history: it predates this contract and hosts target it today. | +| `rse-cmd-bar` | `CmdBar` card root (`src/components/cmd_bar/index.js`) — the whole toolbar card, not one row | The toolbar's outlined selects are compressed to 30px, so their shrunk `InputLabel` floats above its own box. A host that bounds the editor must not clip this card. | +| `rse-lcms-stack` | LC/MS stack root (`src/components/d3_line_rect/index.js`) | The one place the editor mounts three chart containers (`.d3Line`, `.d3Multi`, `.d3Rect`) stacked in a single pane rather than one. A blanket `.d3Line { height: 100% }` rule written for the single-chart layouts triples this stack's height. | +| `rse-lcms-graph-panel` | m/z pane wrapper inside the stack | Wraps `.d3Rect` plus the loading overlay, so it — not `.d3Rect` — is the flex item beside the other two charts. | + +New hooks are `rse-` prefixed: they land in a host's global, non-modular stylesheet next to +its own classes, so a generic name would be one collision away from the host's own markup — +and the no-rename rule above makes that expensive to undo. `react-spectrum-editor` keeps its +historical name because hosts already depend on it. The chart mount classes themselves (`d3Line` / `d3Multi` / `d3Rect` and the inner `d3Svg` / `d3SvgMulti` / `d3SvgRect`) are already stable (`LIST_ROOT_SVG_GRAPH`, diff --git a/src/__tests__/units/components/host_dom_hooks.test.js b/src/__tests__/units/components/host_dom_hooks.test.js index bd023298..afc93320 100644 --- a/src/__tests__/units/components/host_dom_hooks.test.js +++ b/src/__tests__/units/components/host_dom_hooks.test.js @@ -10,23 +10,52 @@ import CmdBar from '../../../components/cmd_bar/index'; import ViewerLineRect from '../../../components/d3_line_rect/index'; import { LIST_HOST_HOOK_CLASS } from '../../../constants/list_graph'; -// chemotion_ELN styles the editor from its own stylesheet and can only target class -// names that survive a build - withStyles emits opaque `jss8 jss4` ones. These hooks are +// chemotion_ELN styles the editor from its own stylesheet and can only target class names +// that survive a build - withStyles emits opaque `jss8 jss4` ones. These hooks are // therefore a published contract: a rename silently breaks the host's layout (the LC/MS // three-chart stack collapsing to its first pane, the toolbar's floating select labels -// getting clipped), with nothing failing here. Assert on the literal strings so a rename -// has to be a deliberate, host-coordinated change. +// getting clipped), with nothing failing here. describe('host DOM hooks', () => { - it('pins the published class names', () => { - expect(LIST_HOST_HOOK_CLASS).toEqual({ - CMD_BAR: 'rse-cmd-bar', - LCMS_STACK: 'lcms-stack', - LCMS_GRAPH_PANEL: 'lcms-graph-panel', - }); + // d3-tip attaches to the chart svg on mount and calls SVG geometry APIs jsdom does not + // implement. `beforeAll` runs before the first test of the whole describe, not just the + // chart one, and nothing restores the prototype afterwards - so treat stubbed SVG + // geometry as in force for every test in this file. + beforeAll(() => { + const proto = window.SVGSVGElement.prototype; + proto.createSVGPoint = proto.createSVGPoint || (() => ({ + x: 0, + y: 0, + matrixTransform: () => ({ x: 0, y: 0 }), + })); + proto.getScreenCTM = proto.getScreenCTM || (() => ({ + a: 1, + b: 0, + c: 0, + d: 1, + e: 0, + f: 0, + inverse: () => ({ + a: 1, b: 0, c: 0, d: 1, e: 0, f: 0, + }), + })); + }); + + // Asserted per entry rather than with toEqual on the whole object: renaming one of these + // is the breaking change worth gating, while adding a fourth hook is not. + it.each([ + ['EDITOR_ROOT', 'react-spectrum-editor'], + ['CMD_BAR', 'rse-cmd-bar'], + ['LCMS_STACK', 'rse-lcms-stack'], + ['LCMS_GRAPH_PANEL', 'rse-lcms-graph-panel'], + ])('pins the published class name for %s', (key, className) => { + expect(LIST_HOST_HOOK_CLASS[key]).toEqual(className); }); // The editor's own store, so every slice these connected components select from is - // present without hand-rolling a fixture of the whole state tree. + // present without hand-rolling a fixture of the whole state tree. Unlike the rest of the + // component suite (which uses redux-mock-store), this one boots the real store because + // ViewerLineRect selects from six slices; the trade-off is that it is shared mutable + // state, so the assertions below are on rendered DOM only, never on store contents. const theme = createTheme(); const withStore = (ui) => render( @@ -41,26 +70,12 @@ describe('host DOM hooks', () => { hasEdit={false} forecast={{}} operations={[]} - editorOnly={true} + editorOnly />, ); expect(container.querySelector(`.${LIST_HOST_HOOK_CLASS.CMD_BAR}`)).toBeInTheDocument(); }); - // d3-tip attaches to the chart svg on mount and calls SVG geometry APIs jsdom does - // not implement. Only this suite renders a real chart, so the shims stay local. - beforeAll(() => { - const proto = window.SVGSVGElement.prototype; - proto.createSVGPoint = proto.createSVGPoint || (() => ({ - x: 0, - y: 0, - matrixTransform: () => ({ x: 0, y: 0 }), - })); - proto.getScreenCTM = proto.getScreenCTM || (() => ({ - a: 1, b: 0, c: 0, d: 1, e: 0, f: 0, inverse: () => ({ a: 1, b: 0, c: 0, d: 1, e: 0, f: 0 }), - })); - }); - it('marks the LC/MS stack root and the m/z pane', () => { const { container } = withStore( { mzEntities={[]} feature={{}} jcampIdx={0} - hplcMsSt={store.getState().hplcMs} isHidden={false} />, ); const stack = container.querySelector(`.${LIST_HOST_HOOK_CLASS.LCMS_STACK}`); expect(stack).toBeInTheDocument(); // The three chart mounts the host's height rules act on must all be inside the stack, - // otherwise a host rule scoped to `.lcms-stack` misses one of them. + // otherwise a host rule scoped to the stack misses one of them. expect(stack.querySelector('.d3Line')).toBeInTheDocument(); expect(stack.querySelector('.d3Multi')).toBeInTheDocument(); const panel = stack.querySelector(`.${LIST_HOST_HOOK_CLASS.LCMS_GRAPH_PANEL}`); diff --git a/src/app.js b/src/app.js index 8017f46e..f6e86d97 100644 --- a/src/app.js +++ b/src/app.js @@ -13,6 +13,7 @@ import reducers from './reducers/index'; import sagas from './sagas/index'; import LayerInit from './layer_init'; import FN from './fn'; +import { LIST_HOST_HOOK_CLASS } from './constants/list_graph'; // - - - store & middleware - - - const sagaMiddleware = createSagaMiddleware(); @@ -118,4 +119,9 @@ SpectraEditor.defaultProps = { export { SpectraEditor, FN, store, + // Published DOM hook class names - see docs/architecture/frontend-architecture.md. + // Exported here so a host can reference them programmatically instead of deep-importing + // dist/constants/list_graph or hard-coding the literals, which is the coupling the + // constant exists to remove. + LIST_HOST_HOOK_CLASS, }; diff --git a/src/components/d3_line_rect/index.js b/src/components/d3_line_rect/index.js index c9836419..14d3e19b 100644 --- a/src/components/d3_line_rect/index.js +++ b/src/components/d3_line_rect/index.js @@ -92,14 +92,32 @@ const styles = () => ( lcMsStackRoot: { margin: '0 0 5px 52px', // This is the only place the editor mounts three chart containers stacked in one - // pane instead of a single one. A host stylesheet that stretches a single chart with - // `.d3Line { height: 100% }` would otherwise make each of the three as tall as - // this whole pane and push the TIC and m/z graphs out of a bounded, clipped - // container. As a flex column the same declaration becomes a flex-basis the - // three panes negotiate down to a third each, so the stack fits either way. + // pane instead of a single one. A host stylesheet that stretches a single chart + // with `.d3Line { height: 100% }` would otherwise make each of the three as tall + // as this whole pane and push the TIC and m/z graphs out of a bounded, clipped + // container. As a flex column they share the pane instead, so the stack fits + // whether or not the host ships such a rule. display: 'flex', flexDirection: 'column', + // Load-bearing under a host that makes this node a flex item (chemotion_ELN's + // `.MuiGrid-grid-xs-9 { display: flex; flex-direction: column }` does), where the + // initial `min-height: auto` would otherwise resolve to the stack's content size + // and defeat the shrink below. minHeight: 0, + // All three panes need the SAME flex-basis or the deficit is shared in proportion + // to basis and one of them is squeezed to a fraction of a third. `height: 100%` + // rather than `flex-basis: 0`, because a host stylesheet loads after this JSS and + // chemotion_ELN already puts `height: 100%` on the two bare mounts with a higher + // specificity than anything reachable from here - a basis of 0 would lose that + // contest on `.d3Line`/`.d3Multi`, apply to the m/z panel alone, and collapse it to + // zero. Restating the same declaration agrees with such a host and supplies it for + // one that bounds our height without styling the mounts itself. Against an + // unbounded parent it computes to `auto`, i.e. the content height, as before. + '& > .d3Line, & > .d3Multi': { + flex: '1 1 auto', + minHeight: 0, + height: '100%', + }, }, lcMsToolbarRow: { // Never absorb the shrink the chart panes above negotiate. @@ -131,9 +149,11 @@ const styles = () => ( }, lcMsGraphPanel: { position: 'relative', - // Wraps the m/z chart, so it must flex like the two bare chart mounts beside it. + // Wraps the m/z chart plus its loading overlay, so this - not `.d3Rect` - is the + // flex item beside the two bare mounts, and takes the same basis as they do. flex: '1 1 auto', minHeight: 0, + height: '100%', }, lcMsLoadingOverlay: { position: 'absolute', diff --git a/src/components/hplc_viewer.js b/src/components/hplc_viewer.js index 6450884c..c49834cc 100644 --- a/src/components/hplc_viewer.js +++ b/src/components/hplc_viewer.js @@ -15,6 +15,7 @@ import CmdBar from './cmd_bar/index'; import ViewerLineRect from './d3_line_rect/index'; import LcMsUvToolsBar from './lc_ms_uv_tools_bar'; import { splitAndReindexEntities } from '../helpers/extractEntityLCMS'; +import { LIST_HOST_HOOK_CLASS } from '../constants/list_graph'; const styles = () => ({ root: { @@ -60,7 +61,7 @@ class HPLCViewer extends React.Component { // eslint-disable-line hideMainEditTools={true} prependLcMsToolbar={} /> -
+
({ root: { @@ -34,7 +35,10 @@ const styles = () => ({ fontSize: '14px', }, cvEditor: { - height: 'calc(90vh - 220px)', + // 230, not 220: `commonStyle.card` reserves 10px above the toolbar for the + // outlined selects' floating labels, and this constant is the budget left + // over after it. Keep the two in step. + height: 'calc(90vh - 230px)', display: 'flex', flexDirection: 'column', minHeight: 0, @@ -111,7 +115,7 @@ class MultiJcampsViewer extends React.Component { // eslint-disable-line editorOnly={editorOnly} hideThreshold={!Format.isNmrLayout(layoutSt)} /> -
+
diff --git a/src/components/panel/index.js b/src/components/panel/index.js index 214d9afe..c8db31f8 100644 --- a/src/components/panel/index.js +++ b/src/components/panel/index.js @@ -45,7 +45,8 @@ const styles = () => ({ // applies in a host that does not constrain us (and now actually works). height: '100%', minHeight: 0, - maxHeight: 'calc(90vh - 220px)', // ROI + // 230, not 220 - see the matching constant in multi_jcamps_viewer.js. + maxHeight: 'calc(90vh - 230px)', // ROI overflowX: 'hidden', overflowY: 'auto', margin: '5px 0 0 0', diff --git a/src/constants/list_graph.js b/src/constants/list_graph.js index c291b797..de32ca9b 100644 --- a/src/constants/list_graph.js +++ b/src/constants/list_graph.js @@ -15,15 +15,19 @@ const LIST_BRUSH_SVG_GRAPH = { // between builds, so a host stylesheet has nothing else to target. Treat these as part // of the public DOM contract: do not rename them without a host-side change. // -// Why hosts need them: LCMS_STACK marks the one place the editor mounts three sibling -// chart containers (`.d3Line` + `.d3Multi` + `.d3Rect`) rather than a single one, so a -// blanket `.d3Line { height: 100% }` rule triples the stack's height; CMD_BAR marks the -// toolbar row, whose outlined select labels float above their own box and must not be -// clipped by a host `overflow: hidden`. +// All entries are `rse-` prefixed. These land in a host's global, non-modular stylesheet +// alongside its own classes, so an unprefixed generic name like `lcms-stack` would be one +// collision away from a host's own LC/MS markup - and the contract above makes such a name +// expensive to change afterwards. +// +// EDITOR_ROOT is the oldest of these and the one hosts already target; it is listed here +// so it is covered by the same contract as the rest, rather than living on as a bare +// string literal in four components. const LIST_HOST_HOOK_CLASS = { + EDITOR_ROOT: 'react-spectrum-editor', CMD_BAR: 'rse-cmd-bar', - LCMS_STACK: 'lcms-stack', - LCMS_GRAPH_PANEL: 'lcms-graph-panel', + LCMS_STACK: 'rse-lcms-stack', + LCMS_GRAPH_PANEL: 'rse-lcms-graph-panel', }; export { diff --git a/src/layer_prism.js b/src/layer_prism.js index e3146577..fc043c1a 100644 --- a/src/layer_prism.js +++ b/src/layer_prism.js @@ -14,6 +14,7 @@ import CmdBar from './components/cmd_bar/index'; import LayerContent from './layer_content'; import { LIST_UI_VIEWER_TYPE } from './constants/list_ui'; import { extractParams } from './helpers/extractParams'; +import { LIST_HOST_HOOK_CLASS } from './constants/list_graph'; const styles = () => ({ }); @@ -55,7 +56,7 @@ const LayerPrism = ({ operations={operations} editorOnly={editorOnly} /> -
+
-
+
Date: Tue, 25 Aug 2026 19:58:04 +0200 Subject: [PATCH 4/6] fix(lint): wrap the editor-root className call to satisfy max-len Introduced when the bare 'react-spectra-editor' literal was replaced with LIST_HOST_HOOK_CLASS.EDITOR_ROOT, which pushed the line to 105 characters. Caught by CI's cypress job, not by 'yarn test': the production build runs eslint and fails on it, while the unit-test command does not lint at all. Verified here by running the same production build rather than eslint alone, whose standalone config cannot parse this codebase's optional chaining. --- src/components/multi_jcamps_viewer.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/components/multi_jcamps_viewer.js b/src/components/multi_jcamps_viewer.js index 400b1636..90b2c05e 100644 --- a/src/components/multi_jcamps_viewer.js +++ b/src/components/multi_jcamps_viewer.js @@ -115,7 +115,12 @@ class MultiJcampsViewer extends React.Component { // eslint-disable-line editorOnly={editorOnly} hideThreshold={!Format.isNmrLayout(layoutSt)} /> -
+
From d7a005bf546668626523821e7f8a7a895c4372fc Mon Sep 17 00:00:00 2001 From: PiTrem Date: Wed, 26 Aug 2026 10:12:55 +0200 Subject: [PATCH 5/6] fix(lcms): fill the pane width, and put the m/z threshold with the other controls Two reports against the LC/MS layout. The threshold buttons above the m/z graph sat in the toolbar row's right cluster, alone, while every other control in all three rows sits left. That put them far from the buttons they belong with and nowhere near the graph they act on. Moved into the left cluster beside that pane's own zoom control; the right cluster is now empty, matching the two rows above. The graphs also left part of the pane unused, more so the wider the viewport. Each chart svg carries preserveAspectRatio="xMinYMin meet", so the viewBox aspect - not the container - decides how much of the pane the drawing fills, and the viewBox came from a W/H pair computed once from window.innerWidth at module load. Whenever the real pane is proportionally wider than that ratio, the drawing scales down to the pane height and the surplus width is left empty on the right. Measured in Chrome against the host's DOM chain, with the stack sized the way the modal sizes it, the waste was 17% at 1920x1080, 17% at 2560x1080 and 6% at 3440x1440 - so this was never only an ultrawide problem, just most visible there. Note it is driven by the pane being shorter than the H estimate, which depends on the host's modal chrome; the percentages carry that assumption, the mechanism does not. Measure each pane and give its svg a matching viewBox, remounting on resize - the pattern d3_multi already uses for the Cyclic Voltammetry layout, which is the only layout that measured its container until now. Drawn width is then the full pane width in every case above. Falls back to the previous constants when a pane cannot be measured, which keeps jsdom (clientWidth 0) and any pre-paint call on exactly today's behaviour. Only the first mount resets redux; a resize must not discard the user's zoom, threshold or selection. Redraw is guarded on a real size change, so against an unbounded host - where the measured size is the one the current viewBox already produces - it settles after the first pass instead of feeding itself. The layout effect itself is not unit-testable (jsdom reports no geometry), so the tests cover the measurement logic and its fallback, and the fill claim rests on the browser measurement above. --- dist/components/d3_line_rect/index.js | 338 +++++++++++------- .../units/components/d3_line_rect.test.js | 59 ++- src/components/d3_line_rect/index.js | 318 ++++++++++------ 3 files changed, 482 insertions(+), 233 deletions(-) diff --git a/dist/components/d3_line_rect/index.js b/dist/components/d3_line_rect/index.js index 0aa54664..d7dd34eb 100644 --- a/dist/components/d3_line_rect/index.js +++ b/dist/components/d3_line_rect/index.js @@ -4,7 +4,7 @@ var _interopRequireDefault = require("@babel/runtime/helpers/interopRequireDefau Object.defineProperty(exports, "__esModule", { value: true }); -exports.isLcmsMsPageLoading = exports.default = void 0; +exports.sameSizes = exports.measurePane = exports.isLcmsMsPageLoading = exports.default = void 0; var _react = _interopRequireDefault(require("react")); var _reactRedux = require("react-redux"); var _redux = require("redux"); @@ -40,9 +40,37 @@ var _extractEntityLCMS = require("../../helpers/extractEntityLCMS"); var _jsxRuntime = require("react/jsx-runtime"); /* eslint-disable no-mixed-operators, prefer-object-spread, react/function-component-definition */ +// Fallback viewBox, used only until the panes can be measured (and in jsdom, where +// clientWidth/clientHeight are 0). const W = Math.round(window.innerWidth * 0.90 * 9 / 12); // ROI const H = Math.round(window.innerHeight * 0.90 * 0.8 / 3); // ROI +// Below this, the drawable area net of the focus classes' margins (l:60 r:5 t:5 b:40) +// stops being meaningful; clamp rather than let a scale range invert. +const MIN_PANE_W = 240; +const MIN_PANE_H = 96; + +// Each chart svg carries `preserveAspectRatio="xMinYMin meet"`, so it is the viewBox +// aspect - not the container - that decides how much of the pane the drawing fills. With +// a viewBox derived once from `window.innerWidth` at module load, a pane proportionally +// wider than that ratio scales the drawing down to its height and leaves the surplus +// width empty on the right, which is what a viewport wider than FHD produces. Measuring +// the pane and matching the viewBox to it removes the letterboxing in both directions. +const measurePane = node => { + if (!node) return null; + const { + clientWidth, + clientHeight + } = node; + if (!clientWidth || !clientHeight) return null; + return { + width: Math.max(Math.round(clientWidth), MIN_PANE_W), + height: Math.max(Math.round(clientHeight), MIN_PANE_H) + }; +}; +exports.measurePane = measurePane; +const sameSizes = (a, b) => Boolean(a) && Boolean(b) && ['line', 'multi', 'rect'].every(k => a[k].width === b[k].width && a[k].height === b[k].height); +exports.sameSizes = sameSizes; const toSeed = (xValues = [], yValues = []) => { const maxLength = Math.min(xValues.length, yValues.length); const seed = new Array(maxLength); @@ -286,46 +314,20 @@ const ticSelect = (classes, hplcMsSt, handleTicChanged) => { class ViewerLineRect extends _react.default.Component { constructor(props) { super(props); - const { - clickUiTargetAct, - selectUiSweepAct, - scrollUiWheelAct, - ticEntities, - uvvisEntities, - uiSt - } = props; this.rootKlassLine = `.${_list_graph.LIST_ROOT_SVG_GRAPH.LINE}`; - this.lineFocus = new _line_focus.default({ - W, - H, - uvvisEntities, - clickUiTargetAct, - selectUiSweepAct, - scrollUiWheelAct, - graphIndex: 0, - uiSt - }); this.rootKlassMulti = `.${_list_graph.LIST_ROOT_SVG_GRAPH.MULTI}`; - this.multiFocus = new _multi_focus.default({ - W, - H, - ticEntities, - clickUiTargetAct, - selectUiSweepAct, - scrollUiWheelAct, - graphIndex: 1, - uiSt - }); this.rootKlassRect = `.${_list_graph.LIST_ROOT_SVG_GRAPH.RECT}`; - this.rectFocus = new _rect_focus.default({ - W, - H, - clickUiTargetAct, - selectUiSweepAct, - scrollUiWheelAct, - graphIndex: 2, - uiSt - }); + this.stackRef = /*#__PURE__*/_react.default.createRef(); + this.lineRef = /*#__PURE__*/_react.default.createRef(); + this.multiRef = /*#__PURE__*/_react.default.createRef(); + this.rectRef = /*#__PURE__*/_react.default.createRef(); + this.resizeObserver = null; + this.currentSizes = null; + + // Nothing is mounted yet, so this resolves to the fallback; componentDidMount + // re-measures and rebuilds against the real panes. + this.createFocuses(this.resolvePaneSizes()); + this.handleResize = this.handleResize.bind(this); this.extractSubView = this.extractSubView.bind(this); this.notifyHostOnSubViewerChange = this.notifyHostOnSubViewerChange.bind(this); this.extractUvvisView = this.extractUvvisView.bind(this); @@ -333,80 +335,8 @@ class ViewerLineRect extends _react.default.Component { this.handleUvvisRedo = this.handleUvvisRedo.bind(this); } componentDidMount() { - const { - curveSt, - feature, - ticEntities, - hplcMsSt, - tTrEndPts, - layoutSt, - isUiAddIntgSt, - isUiNoBrushSt, - integrationSt, - isHidden, - resetAllAct, - uiSt, - editPeakSt - } = this.props; - (0, _draw.drawDestroy)(this.rootKlassMulti); - (0, _draw.drawDestroy)(this.rootKlassLine); - (0, _draw.drawDestroy)(this.rootKlassRect); - resetAllAct(feature); - const { - zoom - } = uiSt; - const { - sweepExtent - } = zoom; - const uvvisViewFeature = this.extractUvvisView(); - let uvvisSeed = []; - if (uvvisViewFeature?.data?.[0]) { - const currentData = uvvisViewFeature.data[0]; - const { - x, - y - } = currentData; - uvvisSeed = toSeed(x, y); - } - (0, _draw.drawMain)(this.rootKlassLine, W, H, _list_graph.LIST_BRUSH_SVG_GRAPH.LINE); - this.lineFocus.create({ - filterSeed: uvvisSeed, - filterPeak: [], - tTrEndPts, - layoutSt, - isUiNoBrushSt: true, - sweepExtentSt: sweepExtent[0], - integrationSt, - isUiAddIntgSt, - editPeakSt, - hplcMsSt - }); - (0, _draw.drawLabel)(this.rootKlassLine, null, 'Minutes', 'Intensity'); - (0, _draw.drawDisplay)(this.rootKlassLine, false); - (0, _draw.drawMain)(this.rootKlassMulti, W, H, _list_graph.LIST_BRUSH_SVG_GRAPH.MULTI); - this.multiFocus.create({ - ticEntities, - curveSt, - hplcMsSt, - tTrEndPts, - layoutSt, - sweepExtentSt: sweepExtent[1], - isUiAddIntgSt, - isUiNoBrushSt - }); - (0, _draw.drawLabel)(this.rootKlassMulti, null, 'Minutes', 'Intensity'); - (0, _draw.drawDisplay)(this.rootKlassMulti, isHidden); - (0, _draw.drawMain)(this.rootKlassRect, W, H, _list_graph.LIST_BRUSH_SVG_GRAPH.RECT); - this.rectFocus.create({ - filterSeed: [], - filterPeak: [], - tTrEndPts, - layoutSt, - isUiNoBrushSt: true, - sweepExtentSt: sweepExtent[2] - }); - (0, _draw.drawLabel)(this.rootKlassRect, null, 'm/z', 'Intensity'); - (0, _draw.drawDisplay)(this.rootKlassRect, false); + this.setupResizeObserver(); + this.mountCharts(this.resolvePaneSizes(), true); } componentDidUpdate(prevProps) { const { @@ -433,7 +363,7 @@ class ViewerLineRect extends _react.default.Component { if (uvvisViewFeature?.data?.[0]) { const hasLineSvg = !!document.querySelector(`${this.rootKlassLine} .${_list_graph.LIST_BRUSH_SVG_GRAPH.LINE}`); if (!hasLineSvg) { - (0, _draw.drawMain)(this.rootKlassLine, W, H, _list_graph.LIST_BRUSH_SVG_GRAPH.LINE); + (0, _draw.drawMain)(this.rootKlassLine, this.currentSizes.line.width, this.currentSizes.line.height, _list_graph.LIST_BRUSH_SVG_GRAPH.LINE); } const currentData = uvvisViewFeature.data[0]; const { @@ -462,7 +392,7 @@ class ViewerLineRect extends _react.default.Component { if (this.multiFocus) { const hasMultiSvg = !!document.querySelector(`${this.rootKlassMulti} .${_list_graph.LIST_BRUSH_SVG_GRAPH.MULTI}`); if (!hasMultiSvg) { - (0, _draw.drawMain)(this.rootKlassMulti, W, H, _list_graph.LIST_BRUSH_SVG_GRAPH.MULTI); + (0, _draw.drawMain)(this.rootKlassMulti, this.currentSizes.multi.width, this.currentSizes.multi.height, _list_graph.LIST_BRUSH_SVG_GRAPH.MULTI); } this.multiFocus.update({ curveSt, @@ -497,7 +427,7 @@ class ViewerLineRect extends _react.default.Component { if (subViewFeature) { const hasRectSvg = !!document.querySelector(`${this.rootKlassRect} .${_list_graph.LIST_BRUSH_SVG_GRAPH.RECT}`); if (!hasRectSvg) { - (0, _draw.drawMain)(this.rootKlassRect, W, H, _list_graph.LIST_BRUSH_SVG_GRAPH.RECT); + (0, _draw.drawMain)(this.rootKlassRect, this.currentSizes.rect.width, this.currentSizes.rect.height, _list_graph.LIST_BRUSH_SVG_GRAPH.RECT); } const { threshold @@ -530,10 +460,20 @@ class ViewerLineRect extends _react.default.Component { } } componentWillUnmount() { + this.teardownResizeObserver(); (0, _draw.drawDestroy)(this.rootKlassLine); (0, _draw.drawDestroy)(this.rootKlassMulti); (0, _draw.drawDestroy)(this.rootKlassRect); } + + // Redraw only when a pane actually changed size. Against an unbounded host the measured + // size is the one the current viewBox already produces, so this settles after the first + // pass instead of feeding itself. + handleResize() { + const sizes = this.resolvePaneSizes(); + if (sameSizes(sizes, this.currentSizes)) return; + this.mountCharts(sizes, false); + } handleUvvisUndo() { const { uvvisUndoAct @@ -546,6 +486,153 @@ class ViewerLineRect extends _react.default.Component { } = this.props; uvvisRedoAct(); } + setupResizeObserver() { + if (typeof ResizeObserver === 'undefined') return; + if (!this.stackRef.current || this.resizeObserver) return; + this.resizeObserver = new ResizeObserver(this.handleResize); + this.resizeObserver.observe(this.stackRef.current); + } + + // Measure every pane, so a stack whose three panes differ in height (a host that has + // not equalised them) still gets a correct viewBox each. + resolvePaneSizes() { + const fallback = { + width: W, + height: H + }; + return { + line: measurePane(this.lineRef?.current) || fallback, + multi: measurePane(this.multiRef?.current) || fallback, + rect: measurePane(this.rectRef?.current) || fallback + }; + } + createFocuses(sizes) { + const { + clickUiTargetAct, + selectUiSweepAct, + scrollUiWheelAct, + ticEntities, + uvvisEntities, + uiSt + } = this.props; + const shared = { + clickUiTargetAct, + selectUiSweepAct, + scrollUiWheelAct, + uiSt + }; + this.lineFocus = new _line_focus.default({ + W: sizes.line.width, + H: sizes.line.height, + uvvisEntities, + graphIndex: 0, + ...shared + }); + this.multiFocus = new _multi_focus.default({ + W: sizes.multi.width, + H: sizes.multi.height, + ticEntities, + graphIndex: 1, + ...shared + }); + this.rectFocus = new _rect_focus.default({ + W: sizes.rect.width, + H: sizes.rect.height, + graphIndex: 2, + ...shared + }); + } + teardownResizeObserver() { + if (this.resizeObserver) { + this.resizeObserver.disconnect(); + this.resizeObserver = null; + } + } + + // The whole draw sequence, parameterised by pane size so a resize can re-run it. Only + // the first run resets redux (`shouldReset`); a resize must not discard the user's zoom, + // threshold or selection. + mountCharts(sizes, shouldReset = false) { + const { + curveSt, + feature, + ticEntities, + hplcMsSt, + tTrEndPts, + layoutSt, + isUiAddIntgSt, + isUiNoBrushSt, + integrationSt, + isHidden, + resetAllAct, + uiSt, + editPeakSt + } = this.props; + this.currentSizes = sizes; + (0, _draw.drawDestroy)(this.rootKlassMulti); + (0, _draw.drawDestroy)(this.rootKlassLine); + (0, _draw.drawDestroy)(this.rootKlassRect); + if (shouldReset) { + resetAllAct(feature); + } + this.createFocuses(sizes); + const { + zoom + } = uiSt; + const { + sweepExtent + } = zoom; + const uvvisViewFeature = this.extractUvvisView(); + let uvvisSeed = []; + if (uvvisViewFeature?.data?.[0]) { + const currentData = uvvisViewFeature.data[0]; + const { + x, + y + } = currentData; + uvvisSeed = toSeed(x, y); + } + (0, _draw.drawMain)(this.rootKlassLine, sizes.line.width, sizes.line.height, _list_graph.LIST_BRUSH_SVG_GRAPH.LINE); + this.lineFocus.create({ + filterSeed: uvvisSeed, + filterPeak: [], + tTrEndPts, + layoutSt, + isUiNoBrushSt: true, + sweepExtentSt: sweepExtent[0], + integrationSt, + isUiAddIntgSt, + editPeakSt, + hplcMsSt + }); + (0, _draw.drawLabel)(this.rootKlassLine, null, 'Minutes', 'Intensity'); + (0, _draw.drawDisplay)(this.rootKlassLine, false); + const multiSize = sizes.multi; + (0, _draw.drawMain)(this.rootKlassMulti, multiSize.width, multiSize.height, _list_graph.LIST_BRUSH_SVG_GRAPH.MULTI); + this.multiFocus.create({ + ticEntities, + curveSt, + hplcMsSt, + tTrEndPts, + layoutSt, + sweepExtentSt: sweepExtent[1], + isUiAddIntgSt, + isUiNoBrushSt + }); + (0, _draw.drawLabel)(this.rootKlassMulti, null, 'Minutes', 'Intensity'); + (0, _draw.drawDisplay)(this.rootKlassMulti, isHidden); + (0, _draw.drawMain)(this.rootKlassRect, sizes.rect.width, sizes.rect.height, _list_graph.LIST_BRUSH_SVG_GRAPH.RECT); + this.rectFocus.create({ + filterSeed: [], + filterPeak: [], + tTrEndPts, + layoutSt, + isUiNoBrushSt: true, + sweepExtentSt: sweepExtent[2] + }); + (0, _draw.drawLabel)(this.rootKlassRect, null, 'm/z', 'Intensity'); + (0, _draw.drawDisplay)(this.rootKlassRect, false); + } extractUvvisView() { const { uvvisEntities, @@ -702,6 +789,7 @@ class ViewerLineRect extends _react.default.Component { }; return /*#__PURE__*/(0, _jsxRuntime.jsxs)("div", { className: `${_list_graph.LIST_HOST_HOOK_CLASS.LCMS_STACK} ${classes.lcMsStackRoot}`, + ref: this.stackRef, children: [omitUvvisToolbarRow ? null : /*#__PURE__*/(0, _jsxRuntime.jsxs)("div", { className: classes.lcMsToolbarRow, children: [/*#__PURE__*/(0, _jsxRuntime.jsxs)("div", { @@ -754,7 +842,8 @@ class ViewerLineRect extends _react.default.Component { className: classes.lcMsToolbarRight })] }), /*#__PURE__*/(0, _jsxRuntime.jsx)("div", { - className: _list_graph.LIST_ROOT_SVG_GRAPH.LINE + className: _list_graph.LIST_ROOT_SVG_GRAPH.LINE, + ref: this.lineRef }), /*#__PURE__*/(0, _jsxRuntime.jsxs)("div", { className: classes.lcMsToolbarRow, children: [/*#__PURE__*/(0, _jsxRuntime.jsxs)("div", { @@ -772,23 +861,24 @@ class ViewerLineRect extends _react.default.Component { className: classes.lcMsToolbarRight })] }), /*#__PURE__*/(0, _jsxRuntime.jsx)("div", { - className: _list_graph.LIST_ROOT_SVG_GRAPH.MULTI + className: _list_graph.LIST_ROOT_SVG_GRAPH.MULTI, + ref: this.multiRef }), /*#__PURE__*/(0, _jsxRuntime.jsxs)("div", { className: classes.lcMsToolbarRow, - children: [/*#__PURE__*/(0, _jsxRuntime.jsx)("div", { + children: [/*#__PURE__*/(0, _jsxRuntime.jsxs)("div", { className: classes.lcMsToolbarLeft, - children: zoomView(classes, 2, uiSt, zoomInAct) - }), /*#__PURE__*/(0, _jsxRuntime.jsx)("div", { - className: classes.lcMsToolbarRight, - children: /*#__PURE__*/(0, _jsxRuntime.jsx)(_r03_threshold.default, { + children: [zoomView(classes, 2, uiSt, zoomInAct), /*#__PURE__*/(0, _jsxRuntime.jsx)(_r03_threshold.default, { feature: resolvedFeature, hasEdit: hasEdit - }) + })] + }), /*#__PURE__*/(0, _jsxRuntime.jsx)("div", { + className: classes.lcMsToolbarRight })] }), /*#__PURE__*/(0, _jsxRuntime.jsxs)("div", { className: `${_list_graph.LIST_HOST_HOOK_CLASS.LCMS_GRAPH_PANEL} ${classes.lcMsGraphPanel}`, children: [/*#__PURE__*/(0, _jsxRuntime.jsx)("div", { - className: _list_graph.LIST_ROOT_SVG_GRAPH.RECT + className: _list_graph.LIST_ROOT_SVG_GRAPH.RECT, + ref: this.rectRef }), isMsLoading ? /*#__PURE__*/(0, _jsxRuntime.jsx)("div", { className: classes.lcMsLoadingOverlay, "data-testid": "lcms-ms-loading", diff --git a/src/__tests__/units/components/d3_line_rect.test.js b/src/__tests__/units/components/d3_line_rect.test.js index eda46908..309ebc06 100644 --- a/src/__tests__/units/components/d3_line_rect.test.js +++ b/src/__tests__/units/components/d3_line_rect.test.js @@ -1,4 +1,4 @@ -import { isLcmsMsPageLoading } from '../../../components/d3_line_rect/index'; +import { isLcmsMsPageLoading, measurePane, sameSizes } from '../../../components/d3_line_rect/index'; import { pickTicIndex } from '../../../components/d3_line_rect/multi_focus'; import RectFocus from '../../../components/d3_line_rect/rect_focus'; @@ -83,3 +83,60 @@ describe('RectFocus.drawBar with an empty threshold-endpoint list (B7)', () => { expect(() => rf.drawBar()).not.toThrow(); }); }); + +// The letterboxing this guards against is a layout effect jsdom cannot observe (it reports +// clientWidth/clientHeight as 0), so what is testable here is the measurement logic and its +// fallback. The claim that the panes actually fill their width rests on browser +// measurement, recorded in the commit message. +describe('measurePane (LC/MS pane sizing)', () => { + it('returns null for a missing node, so callers fall back to the fixed viewBox', () => { + expect(measurePane(null)).toBeNull(); + expect(measurePane(undefined)).toBeNull(); + }); + + it('returns null for an unlaid-out node rather than a degenerate 0x0 viewBox', () => { + // This is the jsdom case, and also a pane measured before first paint. + expect(measurePane({ clientWidth: 0, clientHeight: 0 })).toBeNull(); + expect(measurePane({ clientWidth: 800, clientHeight: 0 })).toBeNull(); + }); + + it('measures a laid-out pane', () => { + expect(measurePane({ clientWidth: 1296, clientHeight: 197 })) + .toEqual({ width: 1296, height: 197 }); + }); + + it('rounds sub-pixel box metrics', () => { + expect(measurePane({ clientWidth: 1295.6, clientHeight: 196.4 })) + .toEqual({ width: 1296, height: 196 }); + }); + + it('clamps below the focus classes own margins, where a scale range would invert', () => { + // margins are l:60 r:5 t:5 b:40, so an unclamped 40x20 pane yields a negative + // drawable width and height. + expect(measurePane({ clientWidth: 40, clientHeight: 20 })) + .toEqual({ width: 240, height: 96 }); + }); +}); + +describe('sameSizes (resize guard)', () => { + const sizes = (w, h) => ({ + line: { width: w, height: h }, + multi: { width: w, height: h }, + rect: { width: w, height: h }, + }); + + it('treats a null previous size as different, so the first measure always mounts', () => { + expect(sameSizes(sizes(100, 50), null)).toBe(false); + }); + + it('is true for identical sizes, which is what stops a resize feedback loop', () => { + expect(sameSizes(sizes(1296, 197), sizes(1296, 197))).toBe(true); + }); + + it('detects a change in any single pane', () => { + const a = sizes(1296, 197); + const b = sizes(1296, 197); + b.rect = { width: 1296, height: 198 }; + expect(sameSizes(a, b)).toBe(false); + }); +}); diff --git a/src/components/d3_line_rect/index.js b/src/components/d3_line_rect/index.js index 14d3e19b..5286e394 100644 --- a/src/components/d3_line_rect/index.js +++ b/src/components/d3_line_rect/index.js @@ -46,9 +46,37 @@ import Integration from '../cmd_bar/04_integration'; import Peak from '../cmd_bar/03_peak'; import { getLcMsInfo } from '../../helpers/extractEntityLCMS'; +// Fallback viewBox, used only until the panes can be measured (and in jsdom, where +// clientWidth/clientHeight are 0). const W = Math.round(window.innerWidth * 0.90 * 9 / 12); // ROI const H = Math.round(window.innerHeight * 0.90 * 0.8 / 3); // ROI +// Below this, the drawable area net of the focus classes' margins (l:60 r:5 t:5 b:40) +// stops being meaningful; clamp rather than let a scale range invert. +const MIN_PANE_W = 240; +const MIN_PANE_H = 96; + +// Each chart svg carries `preserveAspectRatio="xMinYMin meet"`, so it is the viewBox +// aspect - not the container - that decides how much of the pane the drawing fills. With +// a viewBox derived once from `window.innerWidth` at module load, a pane proportionally +// wider than that ratio scales the drawing down to its height and leaves the surplus +// width empty on the right, which is what a viewport wider than FHD produces. Measuring +// the pane and matching the viewBox to it removes the letterboxing in both directions. +export const measurePane = (node) => { + if (!node) return null; + const { clientWidth, clientHeight } = node; + if (!clientWidth || !clientHeight) return null; + return { + width: Math.max(Math.round(clientWidth), MIN_PANE_W), + height: Math.max(Math.round(clientHeight), MIN_PANE_H), + }; +}; + +export const sameSizes = (a, b) => Boolean(a) && Boolean(b) + && ['line', 'multi', 'rect'].every((k) => ( + a[k].width === b[k].width && a[k].height === b[k].height + )); + const toSeed = (xValues = [], yValues = []) => { const maxLength = Math.min(xValues.length, yValues.length); const seed = new Array(maxLength); @@ -299,44 +327,22 @@ class ViewerLineRect extends React.Component { constructor(props) { super(props); - const { - clickUiTargetAct, - selectUiSweepAct, - scrollUiWheelAct, - ticEntities, - uvvisEntities, - uiSt, - } = props; - this.rootKlassLine = `.${LIST_ROOT_SVG_GRAPH.LINE}`; - this.lineFocus = new LineFocus({ - W, - H, - uvvisEntities, - clickUiTargetAct, - selectUiSweepAct, - scrollUiWheelAct, - graphIndex: 0, - uiSt, - }); - this.rootKlassMulti = `.${LIST_ROOT_SVG_GRAPH.MULTI}`; - this.multiFocus = new MultiFocus({ - W, - H, - ticEntities, - clickUiTargetAct, - selectUiSweepAct, - scrollUiWheelAct, - graphIndex: 1, - uiSt, - }); - this.rootKlassRect = `.${LIST_ROOT_SVG_GRAPH.RECT}`; - this.rectFocus = new RectFocus({ - W, H, clickUiTargetAct, selectUiSweepAct, scrollUiWheelAct, graphIndex: 2, uiSt, - }); + this.stackRef = React.createRef(); + this.lineRef = React.createRef(); + this.multiRef = React.createRef(); + this.rectRef = React.createRef(); + this.resizeObserver = null; + this.currentSizes = null; + + // Nothing is mounted yet, so this resolves to the fallback; componentDidMount + // re-measures and rebuilds against the real panes. + this.createFocuses(this.resolvePaneSizes()); + + this.handleResize = this.handleResize.bind(this); this.extractSubView = this.extractSubView.bind(this); this.notifyHostOnSubViewerChange = this.notifyHostOnSubViewerChange.bind(this); this.extractUvvisView = this.extractUvvisView.bind(this); @@ -345,71 +351,8 @@ class ViewerLineRect extends React.Component { } componentDidMount() { - const { - curveSt, feature, ticEntities, hplcMsSt, - tTrEndPts, layoutSt, - isUiAddIntgSt, isUiNoBrushSt, - integrationSt, - isHidden, - resetAllAct, uiSt, - editPeakSt, - } = this.props; - drawDestroy(this.rootKlassMulti); - drawDestroy(this.rootKlassLine); - drawDestroy(this.rootKlassRect); - resetAllAct(feature); - - const { zoom } = uiSt; - const { sweepExtent } = zoom; - - const uvvisViewFeature = this.extractUvvisView(); - let uvvisSeed = []; - if (uvvisViewFeature?.data?.[0]) { - const currentData = uvvisViewFeature.data[0]; - const { x, y } = currentData; - uvvisSeed = toSeed(x, y); - } - drawMain(this.rootKlassLine, W, H, LIST_BRUSH_SVG_GRAPH.LINE); - this.lineFocus.create({ - filterSeed: uvvisSeed, - filterPeak: [], - tTrEndPts, - layoutSt, - isUiNoBrushSt: true, - sweepExtentSt: sweepExtent[0], - integrationSt, - isUiAddIntgSt, - editPeakSt, - hplcMsSt, - }); - drawLabel(this.rootKlassLine, null, 'Minutes', 'Intensity'); - drawDisplay(this.rootKlassLine, false); - - drawMain(this.rootKlassMulti, W, H, LIST_BRUSH_SVG_GRAPH.MULTI); - this.multiFocus.create({ - ticEntities, - curveSt, - hplcMsSt, - tTrEndPts, - layoutSt, - sweepExtentSt: sweepExtent[1], - isUiAddIntgSt, - isUiNoBrushSt, - }); - drawLabel(this.rootKlassMulti, null, 'Minutes', 'Intensity'); - drawDisplay(this.rootKlassMulti, isHidden); - - drawMain(this.rootKlassRect, W, H, LIST_BRUSH_SVG_GRAPH.RECT); - this.rectFocus.create({ - filterSeed: [], - filterPeak: [], - tTrEndPts, - layoutSt, - isUiNoBrushSt: true, - sweepExtentSt: sweepExtent[2], - }); - drawLabel(this.rootKlassRect, null, 'm/z', 'Intensity'); - drawDisplay(this.rootKlassRect, false); + this.setupResizeObserver(); + this.mountCharts(this.resolvePaneSizes(), true); } componentDidUpdate(prevProps) { @@ -430,7 +373,12 @@ class ViewerLineRect extends React.Component { `${this.rootKlassLine} .${LIST_BRUSH_SVG_GRAPH.LINE}`, ); if (!hasLineSvg) { - drawMain(this.rootKlassLine, W, H, LIST_BRUSH_SVG_GRAPH.LINE); + drawMain( + this.rootKlassLine, + this.currentSizes.line.width, + this.currentSizes.line.height, + LIST_BRUSH_SVG_GRAPH.LINE, + ); } const currentData = uvvisViewFeature.data[0]; const { x, y } = currentData; @@ -459,7 +407,12 @@ class ViewerLineRect extends React.Component { `${this.rootKlassMulti} .${LIST_BRUSH_SVG_GRAPH.MULTI}`, ); if (!hasMultiSvg) { - drawMain(this.rootKlassMulti, W, H, LIST_BRUSH_SVG_GRAPH.MULTI); + drawMain( + this.rootKlassMulti, + this.currentSizes.multi.width, + this.currentSizes.multi.height, + LIST_BRUSH_SVG_GRAPH.MULTI, + ); } this.multiFocus.update({ curveSt, @@ -495,7 +448,12 @@ class ViewerLineRect extends React.Component { `${this.rootKlassRect} .${LIST_BRUSH_SVG_GRAPH.RECT}`, ); if (!hasRectSvg) { - drawMain(this.rootKlassRect, W, H, LIST_BRUSH_SVG_GRAPH.RECT); + drawMain( + this.rootKlassRect, + this.currentSizes.rect.width, + this.currentSizes.rect.height, + LIST_BRUSH_SVG_GRAPH.RECT, + ); } const { threshold } = hplcMsSt; const curTrEndPts = convertThresEndPts(subViewFeature, threshold.value); @@ -529,11 +487,21 @@ class ViewerLineRect extends React.Component { } componentWillUnmount() { + this.teardownResizeObserver(); drawDestroy(this.rootKlassLine); drawDestroy(this.rootKlassMulti); drawDestroy(this.rootKlassRect); } + // Redraw only when a pane actually changed size. Against an unbounded host the measured + // size is the one the current viewBox already produces, so this settles after the first + // pass instead of feeding itself. + handleResize() { + const sizes = this.resolvePaneSizes(); + if (sameSizes(sizes, this.currentSizes)) return; + this.mountCharts(sizes, false); + } + handleUvvisUndo() { const { uvvisUndoAct } = this.props; uvvisUndoAct(); @@ -544,6 +512,138 @@ class ViewerLineRect extends React.Component { uvvisRedoAct(); } + setupResizeObserver() { + if (typeof ResizeObserver === 'undefined') return; + if (!this.stackRef.current || this.resizeObserver) return; + this.resizeObserver = new ResizeObserver(this.handleResize); + this.resizeObserver.observe(this.stackRef.current); + } + + // Measure every pane, so a stack whose three panes differ in height (a host that has + // not equalised them) still gets a correct viewBox each. + resolvePaneSizes() { + const fallback = { width: W, height: H }; + return { + line: measurePane(this.lineRef?.current) || fallback, + multi: measurePane(this.multiRef?.current) || fallback, + rect: measurePane(this.rectRef?.current) || fallback, + }; + } + + createFocuses(sizes) { + const { + clickUiTargetAct, selectUiSweepAct, scrollUiWheelAct, + ticEntities, uvvisEntities, uiSt, + } = this.props; + const shared = { + clickUiTargetAct, selectUiSweepAct, scrollUiWheelAct, uiSt, + }; + + this.lineFocus = new LineFocus({ + W: sizes.line.width, + H: sizes.line.height, + uvvisEntities, + graphIndex: 0, + ...shared, + }); + this.multiFocus = new MultiFocus({ + W: sizes.multi.width, + H: sizes.multi.height, + ticEntities, + graphIndex: 1, + ...shared, + }); + this.rectFocus = new RectFocus({ + W: sizes.rect.width, + H: sizes.rect.height, + graphIndex: 2, + ...shared, + }); + } + + teardownResizeObserver() { + if (this.resizeObserver) { + this.resizeObserver.disconnect(); + this.resizeObserver = null; + } + } + + // The whole draw sequence, parameterised by pane size so a resize can re-run it. Only + // the first run resets redux (`shouldReset`); a resize must not discard the user's zoom, + // threshold or selection. + mountCharts(sizes, shouldReset = false) { + const { + curveSt, feature, ticEntities, hplcMsSt, + tTrEndPts, layoutSt, + isUiAddIntgSt, isUiNoBrushSt, + integrationSt, + isHidden, + resetAllAct, uiSt, + editPeakSt, + } = this.props; + this.currentSizes = sizes; + drawDestroy(this.rootKlassMulti); + drawDestroy(this.rootKlassLine); + drawDestroy(this.rootKlassRect); + if (shouldReset) { + resetAllAct(feature); + } + this.createFocuses(sizes); + + const { zoom } = uiSt; + const { sweepExtent } = zoom; + + const uvvisViewFeature = this.extractUvvisView(); + let uvvisSeed = []; + if (uvvisViewFeature?.data?.[0]) { + const currentData = uvvisViewFeature.data[0]; + const { x, y } = currentData; + uvvisSeed = toSeed(x, y); + } + drawMain(this.rootKlassLine, sizes.line.width, sizes.line.height, LIST_BRUSH_SVG_GRAPH.LINE); + this.lineFocus.create({ + filterSeed: uvvisSeed, + filterPeak: [], + tTrEndPts, + layoutSt, + isUiNoBrushSt: true, + sweepExtentSt: sweepExtent[0], + integrationSt, + isUiAddIntgSt, + editPeakSt, + hplcMsSt, + }); + drawLabel(this.rootKlassLine, null, 'Minutes', 'Intensity'); + drawDisplay(this.rootKlassLine, false); + + const multiSize = sizes.multi; + drawMain(this.rootKlassMulti, multiSize.width, multiSize.height, LIST_BRUSH_SVG_GRAPH.MULTI); + this.multiFocus.create({ + ticEntities, + curveSt, + hplcMsSt, + tTrEndPts, + layoutSt, + sweepExtentSt: sweepExtent[1], + isUiAddIntgSt, + isUiNoBrushSt, + }); + drawLabel(this.rootKlassMulti, null, 'Minutes', 'Intensity'); + drawDisplay(this.rootKlassMulti, isHidden); + + drawMain(this.rootKlassRect, sizes.rect.width, sizes.rect.height, LIST_BRUSH_SVG_GRAPH.RECT); + this.rectFocus.create({ + filterSeed: [], + filterPeak: [], + tTrEndPts, + layoutSt, + isUiNoBrushSt: true, + sweepExtentSt: sweepExtent[2], + }); + drawLabel(this.rootKlassRect, null, 'm/z', 'Intensity'); + drawDisplay(this.rootKlassRect, false); + } + extractUvvisView() { const { uvvisEntities, hplcMsSt } = this.props; if (!uvvisEntities || !uvvisEntities[0]) { @@ -696,7 +796,10 @@ class ViewerLineRect extends React.Component { selectWavelengthAct(event); }; return ( -
+
{ omitUvvisToolbarRow ? null : (
@@ -743,7 +846,7 @@ class ViewerLineRect extends React.Component {
) } -
+
{ @@ -758,19 +861,18 @@ class ViewerLineRect extends React.Component {
-
+
{ zoomView(classes, 2, uiSt, zoomInAct) } -
-
+
-
+
{ isMsLoading ? (
Date: Wed, 26 Aug 2026 10:47:01 +0200 Subject: [PATCH 6/6] fix(lcms): defer the resize remount, and keep the m/z scan across it The pane-measuring resize path failed the whole LC/MS e2e suite. Two distinct bugs, both invisible to the unit tests and to the production build, and both caught only by running cypress. First: "ResizeObserver loop completed with undelivered notifications", which Cypress reports as an uncaught application error rather than a console warning. Remounting synchronously inside the observer callback resizes the very subtree being observed. I had reasoned this was a stable fixed point and it is not - a pane whose height comes from its content takes that height from the svg, whose height comes back from the viewBox the callback is setting. d3_multi does remount synchronously, but its resize path is gated to Cyclic Voltammetry, whose container height is fixed in CSS and so cannot be fed back into; this stack has no such guarantee, and the standalone demo the e2e suite drives is exactly the unbounded case. Defer to the next animation frame so the observer finishes delivery before anything moves, and require a change of more than a pixel before remounting at all, so integer round-trip noise cannot retrigger it. Second, uncovered once the first was fixed: the remount recreated the m/z pane with an empty series. That pane is populated by componentDidUpdate, not by the mount path, so a resize wiped the displayed scan and nothing redrew it - the graph went blank, and `.d3Rect .d3SvgRect path[d]` stopped existing. Seed it from the currently selected sub-view instead, which is still empty on first mount and correct on every remount after. Verified by running the LC/MS spec locally in a container (the host's port 3000 is in use by another service, and the spec hardcodes that URL): 7/7 pass, and the full suite is 81/81 across all ten specs, including the Cyclic Voltammetry and HPLC layouts that share the toolbar card and editor root this branch also touches. --- dist/components/d3_line_rect/index.js | 59 +++++++++++++++--- .../units/components/d3_line_rect.test.js | 13 +++- src/components/d3_line_rect/index.js | 62 ++++++++++++++++--- 3 files changed, 117 insertions(+), 17 deletions(-) diff --git a/dist/components/d3_line_rect/index.js b/dist/components/d3_line_rect/index.js index d7dd34eb..5b300b6b 100644 --- a/dist/components/d3_line_rect/index.js +++ b/dist/components/d3_line_rect/index.js @@ -4,7 +4,7 @@ var _interopRequireDefault = require("@babel/runtime/helpers/interopRequireDefau Object.defineProperty(exports, "__esModule", { value: true }); -exports.sameSizes = exports.measurePane = exports.isLcmsMsPageLoading = exports.default = void 0; +exports.sameSizes = exports.measurePane = exports.isLcmsMsPageLoading = exports.default = exports.SIZE_EPSILON = void 0; var _react = _interopRequireDefault(require("react")); var _reactRedux = require("react-redux"); var _redux = require("redux"); @@ -69,7 +69,13 @@ const measurePane = node => { }; }; exports.measurePane = measurePane; -const sameSizes = (a, b) => Boolean(a) && Boolean(b) && ['line', 'multi', 'rect'].every(k => a[k].width === b[k].width && a[k].height === b[k].height); +// A pane whose height is content-derived (any host that does not bound us - the +// standalone demo included) takes its height from the svg, whose height comes back from +// the viewBox we are about to set. Re-measuring integer client boxes across that round +// trip can differ by a pixel without anything really having moved, so require a real +// change before paying for a remount. +const SIZE_EPSILON = exports.SIZE_EPSILON = 2; +const sameSizes = (a, b) => Boolean(a) && Boolean(b) && ['line', 'multi', 'rect'].every(k => Math.abs(a[k].width - b[k].width) <= SIZE_EPSILON && Math.abs(a[k].height - b[k].height) <= SIZE_EPSILON); exports.sameSizes = sameSizes; const toSeed = (xValues = [], yValues = []) => { const maxLength = Math.min(xValues.length, yValues.length); @@ -322,6 +328,7 @@ class ViewerLineRect extends _react.default.Component { this.multiRef = /*#__PURE__*/_react.default.createRef(); this.rectRef = /*#__PURE__*/_react.default.createRef(); this.resizeObserver = null; + this.resizeFrame = null; this.currentSizes = null; // Nothing is mounted yet, so this resolves to the fallback; componentDidMount @@ -470,9 +477,22 @@ class ViewerLineRect extends _react.default.Component { // size is the one the current viewBox already produces, so this settles after the first // pass instead of feeding itself. handleResize() { - const sizes = this.resolvePaneSizes(); - if (sameSizes(sizes, this.currentSizes)) return; - this.mountCharts(sizes, false); + // Never mutate layout synchronously inside a ResizeObserver callback. The remount + // resizes the subtree being observed, and the browser abandons the delivery pass with + // "ResizeObserver loop completed with undelivered notifications" - which surfaces as + // an uncaught application error, not just a console warning. Deferring to the next + // frame lets the observer finish before anything moves. + // + // d3_multi remounts synchronously and gets away with it because its resize path is + // gated to Cyclic Voltammetry, whose container height is fixed by CSS and so cannot + // be fed back into by a redraw. This stack has no such guarantee. + if (this.resizeFrame != null) return; + this.resizeFrame = window.requestAnimationFrame(() => { + this.resizeFrame = null; + const sizes = this.resolvePaneSizes(); + if (sameSizes(sizes, this.currentSizes)) return; + this.mountCharts(sizes, false); + }); } handleUvvisUndo() { const { @@ -543,6 +563,10 @@ class ViewerLineRect extends _react.default.Component { }); } teardownResizeObserver() { + if (this.resizeFrame != null) { + window.cancelAnimationFrame(this.resizeFrame); + this.resizeFrame = null; + } if (this.resizeObserver) { this.resizeObserver.disconnect(); this.resizeObserver = null; @@ -621,16 +645,35 @@ class ViewerLineRect extends _react.default.Component { }); (0, _draw.drawLabel)(this.rootKlassMulti, null, 'Minutes', 'Intensity'); (0, _draw.drawDisplay)(this.rootKlassMulti, isHidden); + + // Seed the m/z pane with the scan that is currently selected rather than with an + // empty series. On first mount there is none and this stays [] as before - but a + // resize remount happens long after componentDidUpdate has drawn a scan here, and + // recreating the pane empty would silently wipe it with nothing to redraw it. + const subViewFeature = this.extractSubView(); + let subSeed = []; + let subTrEndPts = tTrEndPts; + let subLabel = null; + if (subViewFeature?.data?.[0]) { + const { + x, + y + } = subViewFeature.data[0]; + subSeed = toSeed(x, y); + subTrEndPts = (0, _chem.convertThresEndPts)(subViewFeature, hplcMsSt?.threshold?.value); + const pageValue = (0, _pageValue.parseFeaturePageValue)(subViewFeature); + subLabel = Number.isFinite(pageValue) ? pageValue : subViewFeature?.pageValue ?? subViewFeature?.page ?? null; + } (0, _draw.drawMain)(this.rootKlassRect, sizes.rect.width, sizes.rect.height, _list_graph.LIST_BRUSH_SVG_GRAPH.RECT); this.rectFocus.create({ - filterSeed: [], + filterSeed: subSeed, filterPeak: [], - tTrEndPts, + tTrEndPts: subTrEndPts, layoutSt, isUiNoBrushSt: true, sweepExtentSt: sweepExtent[2] }); - (0, _draw.drawLabel)(this.rootKlassRect, null, 'm/z', 'Intensity'); + (0, _draw.drawLabel)(this.rootKlassRect, subLabel != null ? `${subLabel} min` : null, 'm/z', 'Intensity'); (0, _draw.drawDisplay)(this.rootKlassRect, false); } extractUvvisView() { diff --git a/src/__tests__/units/components/d3_line_rect.test.js b/src/__tests__/units/components/d3_line_rect.test.js index 309ebc06..3cb2e464 100644 --- a/src/__tests__/units/components/d3_line_rect.test.js +++ b/src/__tests__/units/components/d3_line_rect.test.js @@ -133,10 +133,19 @@ describe('sameSizes (resize guard)', () => { expect(sameSizes(sizes(1296, 197), sizes(1296, 197))).toBe(true); }); - it('detects a change in any single pane', () => { + it('detects a real change in any single pane', () => { const a = sizes(1296, 197); const b = sizes(1296, 197); - b.rect = { width: 1296, height: 198 }; + b.rect = { width: 1296, height: 260 }; expect(sameSizes(a, b)).toBe(false); }); + + it('absorbs a one-pixel difference, which is round-trip noise rather than a resize', () => { + // A content-height pane takes its height from the svg, whose height comes back from + // the viewBox this measurement sets. Treating a 1px integer-rounding difference as a + // resize would remount forever. + const a = sizes(1296, 197); + const b = sizes(1297, 198); + expect(sameSizes(a, b)).toBe(true); + }); }); diff --git a/src/components/d3_line_rect/index.js b/src/components/d3_line_rect/index.js index 5286e394..978ea80a 100644 --- a/src/components/d3_line_rect/index.js +++ b/src/components/d3_line_rect/index.js @@ -72,9 +72,17 @@ export const measurePane = (node) => { }; }; +export // A pane whose height is content-derived (any host that does not bound us - the +// standalone demo included) takes its height from the svg, whose height comes back from +// the viewBox we are about to set. Re-measuring integer client boxes across that round +// trip can differ by a pixel without anything really having moved, so require a real +// change before paying for a remount. +const SIZE_EPSILON = 2; + export const sameSizes = (a, b) => Boolean(a) && Boolean(b) && ['line', 'multi', 'rect'].every((k) => ( - a[k].width === b[k].width && a[k].height === b[k].height + Math.abs(a[k].width - b[k].width) <= SIZE_EPSILON + && Math.abs(a[k].height - b[k].height) <= SIZE_EPSILON )); const toSeed = (xValues = [], yValues = []) => { @@ -336,6 +344,7 @@ class ViewerLineRect extends React.Component { this.multiRef = React.createRef(); this.rectRef = React.createRef(); this.resizeObserver = null; + this.resizeFrame = null; this.currentSizes = null; // Nothing is mounted yet, so this resolves to the fallback; componentDidMount @@ -497,9 +506,22 @@ class ViewerLineRect extends React.Component { // size is the one the current viewBox already produces, so this settles after the first // pass instead of feeding itself. handleResize() { - const sizes = this.resolvePaneSizes(); - if (sameSizes(sizes, this.currentSizes)) return; - this.mountCharts(sizes, false); + // Never mutate layout synchronously inside a ResizeObserver callback. The remount + // resizes the subtree being observed, and the browser abandons the delivery pass with + // "ResizeObserver loop completed with undelivered notifications" - which surfaces as + // an uncaught application error, not just a console warning. Deferring to the next + // frame lets the observer finish before anything moves. + // + // d3_multi remounts synchronously and gets away with it because its resize path is + // gated to Cyclic Voltammetry, whose container height is fixed by CSS and so cannot + // be fed back into by a redraw. This stack has no such guarantee. + if (this.resizeFrame != null) return; + this.resizeFrame = window.requestAnimationFrame(() => { + this.resizeFrame = null; + const sizes = this.resolvePaneSizes(); + if (sameSizes(sizes, this.currentSizes)) return; + this.mountCharts(sizes, false); + }); } handleUvvisUndo() { @@ -562,6 +584,10 @@ class ViewerLineRect extends React.Component { } teardownResizeObserver() { + if (this.resizeFrame != null) { + window.cancelAnimationFrame(this.resizeFrame); + this.resizeFrame = null; + } if (this.resizeObserver) { this.resizeObserver.disconnect(); this.resizeObserver = null; @@ -631,16 +657,38 @@ class ViewerLineRect extends React.Component { drawLabel(this.rootKlassMulti, null, 'Minutes', 'Intensity'); drawDisplay(this.rootKlassMulti, isHidden); + // Seed the m/z pane with the scan that is currently selected rather than with an + // empty series. On first mount there is none and this stays [] as before - but a + // resize remount happens long after componentDidUpdate has drawn a scan here, and + // recreating the pane empty would silently wipe it with nothing to redraw it. + const subViewFeature = this.extractSubView(); + let subSeed = []; + let subTrEndPts = tTrEndPts; + let subLabel = null; + if (subViewFeature?.data?.[0]) { + const { x, y } = subViewFeature.data[0]; + subSeed = toSeed(x, y); + subTrEndPts = convertThresEndPts(subViewFeature, hplcMsSt?.threshold?.value); + const pageValue = parsePageValue(subViewFeature); + subLabel = Number.isFinite(pageValue) + ? pageValue + : (subViewFeature?.pageValue ?? subViewFeature?.page ?? null); + } drawMain(this.rootKlassRect, sizes.rect.width, sizes.rect.height, LIST_BRUSH_SVG_GRAPH.RECT); this.rectFocus.create({ - filterSeed: [], + filterSeed: subSeed, filterPeak: [], - tTrEndPts, + tTrEndPts: subTrEndPts, layoutSt, isUiNoBrushSt: true, sweepExtentSt: sweepExtent[2], }); - drawLabel(this.rootKlassRect, null, 'm/z', 'Intensity'); + drawLabel( + this.rootKlassRect, + subLabel != null ? `${subLabel} min` : null, + 'm/z', + 'Intensity', + ); drawDisplay(this.rootKlassRect, false); }