Skip to content

Commit 08eb387

Browse files
committed
fix(lcms): resolve review regressions B1, B4–B7 on master
PR #289 shipped the LC/MS feature to master at its pre-review state, so the B1–B7 regressions found reviewing #232 are live on master. This applies the fixes that still apply to master's current code: - B1 (saga_ui): restore peak-add for non-LCMS layouts (NMR/IR/MS) — PEAK_ADD dispatched UPDATE_HPLCMS_PEAKS unconditionally, silently swallowing the add - B4 (chem Convert2Peak): honour stored LC/MS feature.peaks (with offset) before recomputing peaks from raw data - B5 (CV current density): apply the mm² area conversion once, not twice (was 100x too small) — fixed in the chart (multi_focus), panel (cyclic_voltamery_data) and submit/export (r05_submit_btn) factors - B6 (d3_line_rect): guard the UV-Vis viewer update path against a featureless feature - B7 (rect_focus): guard drawBar against an empty threshold-endpoint list Not included — B2/B3 (integration getArea/getAbsoluteArea): #303 rewrote the integration helpers on master and they already compute the area from the k-difference / raw y correctly, so that regression is not present there. Adds regression tests for B1, B4, B5 (chart + submit/export), B6, B7. refs #232, #289
1 parent d7148d9 commit 08eb387

12 files changed

Lines changed: 168 additions & 34 deletions

File tree

src/__tests__/units/components/cmd_bar/r05_submit_btn.test.js

Lines changed: 36 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import { Provider } from 'react-redux';
33
import { fireEvent, render } from '@testing-library/react';
44
import '@testing-library/jest-dom';
55

6-
import BtnSubmit from '../../../../components/cmd_bar/r05_submit_btn';
6+
import BtnSubmit, { computeCvYScaleFactor } from '../../../../components/cmd_bar/r05_submit_btn';
77
import Format from '../../../../helpers/format';
88

99
jest.mock('../../../../helpers/extractPeaksEdit', () => ({
@@ -186,4 +186,39 @@ describe('<BtnSubmit payload contract />', () => {
186186
expect(payload.lcms_peaks).toEqual([{ peakMock: true }]);
187187
});
188188
});
189+
190+
// Regression for review finding RR-1 (B5 remainder): the CV current-density factor
191+
// used on submit/export must match the chart (multi_focus.computeYTransformFactor)
192+
// and panel — i.e. one conversion to A/cm², not a double /100 for mm².
193+
describe('computeCvYScaleFactor — CV current-density factor (RR-1 / B5)', () => {
194+
const feature = { yUnit: 'A' };
195+
196+
it('returns 1.0 when current density is off', () => {
197+
expect(computeCvYScaleFactor(feature, { useCurrentDensity: false })).toEqual(1.0);
198+
});
199+
200+
it('treats 100 mm² the same as 1 cm² (no double /100)', () => {
201+
const fMm2 = computeCvYScaleFactor(feature, {
202+
useCurrentDensity: true, areaValue: 100, areaUnit: 'mm²',
203+
});
204+
const fCm2 = computeCvYScaleFactor(feature, {
205+
useCurrentDensity: true, areaValue: 1, areaUnit: 'cm²',
206+
});
207+
expect(fMm2).toBeCloseTo(fCm2);
208+
expect(fMm2).toBeCloseTo(1.0); // pre-fix this was 0.01 (100x too small)
209+
});
210+
211+
it('gives A/cm² for a mm² area (factor = 100 / area_mm²)', () => {
212+
// 50 mm² == 0.5 cm² → factor 1/0.5 = 2
213+
expect(computeCvYScaleFactor(feature, {
214+
useCurrentDensity: true, areaValue: 50, areaUnit: 'mm²',
215+
})).toBeCloseTo(2.0);
216+
});
217+
218+
it('scales by 1000 for mA while keeping the area conversion', () => {
219+
expect(computeCvYScaleFactor({ yUnit: 'mA' }, {
220+
useCurrentDensity: true, areaValue: 100, areaUnit: 'mm²',
221+
})).toBeCloseTo(1000.0);
222+
});
223+
});
189224
});

src/__tests__/units/components/d3_line_rect.test.js

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import { isLcmsMsPageLoading } from '../../../components/d3_line_rect/index';
22
import { pickTicIndex } from '../../../components/d3_line_rect/multi_focus';
3+
import RectFocus from '../../../components/d3_line_rect/rect_focus';
34

45
describe('isLcmsMsPageLoading', () => {
56
const buildMzEntity = (polarity, pageValues) => ({
@@ -66,3 +67,19 @@ describe('isLcmsMsPageLoading', () => {
6667
expect(isLcmsMsPageLoading([], state)).toEqual(true);
6768
});
6869
});
70+
71+
// Review finding B7 (#232): drawBar reads tTrEndPts[0].y. Clearing the
72+
// threshold makes convertThresEndPts return [] (see chem.test.tsx) while the
73+
// MS bars are still present, so drawBar must guard the empty endpoint list
74+
// instead of crashing.
75+
describe('RectFocus.drawBar with an empty threshold-endpoint list (B7)', () => {
76+
it('does not crash when tTrEndPts is empty but bars exist', () => {
77+
const rf = Object.create(RectFocus.prototype);
78+
rf.bars = {}; // truthy → passes the `if (!this.bars)` guard
79+
rf.scales = { x: (v) => v, y: (v) => v }; // TfRescale reads focus.scales.{x,y}
80+
rf.updatePathCall = () => {}; // stub out the d3 path update
81+
rf.data = [{ x: 1, y: 2 }]; // bars present
82+
rf.tTrEndPts = []; // cleared threshold → empty endpoints
83+
expect(() => rf.drawBar()).not.toThrow();
84+
});
85+
});
Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
import MultiFocus from '../../../../components/d3_multi/multi_focus';
2+
import { LIST_LAYOUT } from '../../../../constants/list_layout';
3+
4+
// Review finding B5 (#232): the CV current-density factor on the chart
5+
// (computeYTransformFactor) must convert the electrode area to cm² exactly
6+
// once. Physically 100 mm² == 1 cm², so the factor for the two must match —
7+
// double-dividing by 100 for mm² made it 100x too small.
8+
// (computeYTransformFactor is pure — it ignores `this` — so it is called via
9+
// the prototype.)
10+
describe('MultiFocus.computeYTransformFactor — CV current density (B5)', () => {
11+
const compute = (cvSt) => MultiFocus.prototype.computeYTransformFactor.call(
12+
{}, LIST_LAYOUT.CYCLIC_VOLTAMMETRY, cvSt, { yUnit: 'A' },
13+
);
14+
15+
it('treats 100 mm² the same as 1 cm²', () => {
16+
const fMm2 = compute({ useCurrentDensity: true, areaValue: 100, areaUnit: 'mm²' });
17+
const fCm2 = compute({ useCurrentDensity: true, areaValue: 1, areaUnit: 'cm²' });
18+
expect(fMm2).toBeCloseTo(fCm2);
19+
expect(fMm2).toBeCloseTo(1.0); // double /100 for mm² gives 0.01
20+
});
21+
22+
it('gives A/cm² for a mm² area (factor = 100 / area_mm²)', () => {
23+
// 50 mm² == 0.5 cm² → factor 1 / 0.5 = 2
24+
expect(compute({ useCurrentDensity: true, areaValue: 50, areaUnit: 'mm²' })).toBeCloseTo(2.0);
25+
});
26+
27+
it('returns 1.0 when current density is off', () => {
28+
expect(compute({ useCurrentDensity: false })).toBeCloseTo(1.0);
29+
});
30+
});

src/__tests__/units/helpers/chem.test.tsx

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import {
33
ToFrequency, Convert2Scan, Convert2Thres, GetComparisons, Convert2DValue,
44
GetCyclicVoltaRatio, GetCyclicVoltaPeakSeparate, convertTopic,
55
Convert2MaxMinPeak, Feature2MaxMinPeak, GetCyclicVoltaShiftOffset, GetCyclicVoltaPreviousShift,
6-
buildIntegFeature,
6+
buildIntegFeature, convertThresEndPts,
77
} from "../../../helpers/chem";
88
import nmr1HJcamp from "../../fixtures/nmr1h_jcamp";
99
import aifJcamp1 from "../../fixtures/aif_jcamp_1";
@@ -194,6 +194,29 @@ describe('Test for chem helper', () => {
194194
const peaks = Convert2Peak(feature, threshold, offset)
195195
expect(peaks).toEqual([{x: 2, y: 2}, {x: -2, y: -2}])
196196
})
197+
198+
// Review finding B4 (#232): for an LC/MS feature carrying edited/stored
199+
// peaks, Convert2Peak must return those peaks with the offset applied,
200+
// rather than recomputing them from the raw data and dropping the offset.
201+
it('honours stored LC/MS peaks and applies the offset', () => {
202+
const feature = {
203+
operation: { layout: 'LC/MS' },
204+
data: [{ x: [10, 11, 12], y: [1, 5, 1] }],
205+
peaks: [{ x: 5, y: 100 }], // user-edited / stored peaks
206+
}
207+
const peaks = Convert2Peak(feature, 0, 2)
208+
expect(peaks).toEqual([{ x: 3, y: 100 }])
209+
})
210+
})
211+
212+
// Review finding B7 (#232) precondition: clearing the threshold input yields
213+
// an empty endpoint list (the state that made drawBar crash — the drawBar
214+
// guard itself is covered in components/d3_line_rect.test.js).
215+
describe('convertThresEndPts', () => {
216+
it('returns [] when the threshold is cleared', () => {
217+
const feature = { maxY: 100, maxX: 10, minX: 0, data: [{ x: [1, 2], y: [3, 4] }] }
218+
expect(convertThresEndPts(feature, '')).toEqual([])
219+
})
197220
})
198221

199222
describe('Feature2Peak', () => {

src/__tests__/units/sagas/saga_ui_lcms.test.tsx

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -258,6 +258,33 @@ describe('saga_ui — clickUiTarget LCMS branches', () => {
258258
expect(value).toBeUndefined();
259259
});
260260

261+
// Review finding B1 (#232): on a NON-LCMS layout, PEAK_ADD must still add a
262+
// positive edit-peak (EDITPEAK.ADD_POSITIVE). The LCMS-only path must not
263+
// swallow peak-add for NMR/IR/MS layouts.
264+
it('PEAK_ADD on a non-LCMS layout dispatches EDITPEAK.ADD_POSITIVE', () => {
265+
const action = {
266+
type: UI.CLICK_TARGET,
267+
payload: { x: 5, y: 10 },
268+
sourceHint: null,
269+
onPeak: false,
270+
onPecker: false,
271+
voltammetryPeakIdx: 0,
272+
} as any;
273+
const iter = clickUiTarget(action);
274+
275+
const { value } = drainSelects(iter, [
276+
LIST_UI_SWEEP_TYPE.PEAK_ADD,
277+
{ curveIdx: 0 },
278+
{ uvvis: { selectedWaveLength: null, currentSpectrum: { peaks: [] } } },
279+
LIST_LAYOUT.H1, // a normal NMR layout — not LC/MS
280+
]);
281+
282+
expect(value).toEqual(put({
283+
type: EDITPEAK.ADD_POSITIVE,
284+
payload: { dataToAdd: action.payload, curveIdx: 0 },
285+
}));
286+
});
287+
261288
it('PEAK_DELETE on LCMS uses REMOVE_HPLCMS_PEAK for the selected wavelength', () => {
262289
const action = {
263290
type: UI.CLICK_TARGET,

src/components/cmd_bar/r05_submit_btn.js

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -59,20 +59,18 @@ const buildCvAxisYLabel = (yLabel, cyclicvoltaSt) => {
5959
return `Current in ${baseUnit}`;
6060
};
6161

62-
const computeCvYScaleFactor = (feature, cyclicvoltaSt) => {
62+
export const computeCvYScaleFactor = (feature, cyclicvoltaSt) => {
6363
if (!cyclicvoltaSt?.useCurrentDensity) return 1.0;
6464
const rawArea = (cyclicvoltaSt.areaValue === '' ? 1.0 : cyclicvoltaSt.areaValue) || 1.0;
6565
const areaUnit = cyclicvoltaSt.areaUnit || 'cm²';
6666
const safeArea = rawArea > 0 ? rawArea : 1.0;
67+
// areaInCm2 already converts mm² → cm², so factor is A/cm². Do NOT divide by 100 again.
6768
const areaInCm2 = areaUnit === 'mm²' ? (safeArea / 100.0) : safeArea;
6869
let factor = 1.0 / areaInCm2;
6970
const baseY = feature && feature.yUnit ? String(feature.yUnit) : 'A';
7071
if (/mA/i.test(baseY)) {
7172
factor *= 1000.0;
7273
}
73-
if (areaUnit === 'mm²') {
74-
factor /= 100.0;
75-
}
7674
return factor;
7775
};
7876

src/components/d3_line_rect/index.js

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -389,15 +389,14 @@ class ViewerLineRect extends React.Component {
389389
if (!Array.isArray(sweepExtent)) return;
390390

391391
const uvvisViewFeature = this.extractUvvisView();
392-
if (uvvisViewFeature) {
392+
if (uvvisViewFeature?.data?.[0]) {
393393
const hasLineSvg = !!document.querySelector(
394394
`${this.rootKlassLine} .${LIST_BRUSH_SVG_GRAPH.LINE}`,
395395
);
396396
if (!hasLineSvg) {
397397
drawMain(this.rootKlassLine, W, H, LIST_BRUSH_SVG_GRAPH.LINE);
398398
}
399-
const { data } = uvvisViewFeature;
400-
const currentData = data[0];
399+
const currentData = uvvisViewFeature.data[0];
401400
const { x, y } = currentData;
402401
const uvvisSeed = toSeed(x, y);
403402
if (this.lineFocus) {

src/components/d3_line_rect/rect_focus.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -140,6 +140,7 @@ class RectFocus {
140140
const { xt, yt } = TfRescale(this);
141141
this.updatePathCall(xt, yt);
142142

143+
if (!this.tTrEndPts.length) return;
143144
const yRef = this.tTrEndPts[0].y;
144145
const bars = this.bars.selectAll('rect')
145146
.data(this.data);

src/components/d3_multi/multi_focus.js

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -183,15 +183,13 @@ class MultiFocus {
183183
const rawArea = (cyclicvoltaSt.areaValue === '' ? 1.0 : cyclicvoltaSt.areaValue) || 1.0;
184184
const areaUnit = cyclicvoltaSt.areaUnit || 'cm²';
185185
const safeArea = rawArea > 0 ? rawArea : 1.0;
186+
// areaInCm2 already converts mm² → cm², so factor is A/cm². Do NOT divide by 100 again.
186187
const areaInCm2 = areaUnit === 'mm²' ? (safeArea / 100.0) : safeArea;
187188
factor = 1.0 / areaInCm2;
188189
const baseY = feature && feature.yUnit ? String(feature.yUnit) : 'A';
189190
if (/mA/i.test(baseY)) {
190191
factor *= 1000.0;
191192
}
192-
if (areaUnit === 'mm²') {
193-
factor /= 100.0;
194-
}
195193
}
196194
return factor;
197195
}

src/components/panel/cyclic_voltamery_data.js

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -164,13 +164,14 @@ const CyclicVoltammetryPanel = ({
164164
const rawArea = (cyclicVoltaState && cyclicVoltaState.areaValue === '' ? 1.0 : cyclicVoltaState?.areaValue) || 1.0;
165165
const areaUnit = (cyclicVoltaState && cyclicVoltaState.areaUnit) ? cyclicVoltaState.areaUnit : 'cm²';
166166
const safeArea = rawArea > 0 ? rawArea : 1.0;
167+
const areaInCm2 = areaUnit === 'mm²' ? safeArea / 100.0 : safeArea;
167168

168169
let val = y;
169170
let unit = isMilli ? 'mA' : 'A';
170171

171172
if (useDensity) {
172-
val = y / safeArea;
173-
unit = `${unit}/${areaUnit}`;
173+
val = y / areaInCm2;
174+
unit = `${unit}/cm²`;
174175
}
175176

176177
if (isMilli) {

0 commit comments

Comments
 (0)