From d22739398252399e69596ee8665c1ced13902a97 Mon Sep 17 00:00:00 2001 From: Joe Russack Date: Tue, 28 Jul 2026 16:20:51 -0700 Subject: [PATCH 1/3] fix(locality): do not rewrite the coordinate fields when a record is merely opened LatLongUi re-parses the coordinate and writes the result back inside an effect keyed on the rendered value, so it ran on first render rather than on user edit. The decimal write is silent but the text write is not, so simply opening a Locality marked it dirty; saving it afterwards for any unrelated reason persisted a re-interpretation of the verbatim field over the original. Gate the write-back on an actual change - the user typing, or one of the two resourceOn handlers reacting to a field set elsewhere. Display behaviour (validation message and the formatted Parsed column) is unchanged; only persistence is gated. The flag is reset on [resource, coordinateTextField] because a record selector slides a new resource into the same component instance - useFieldParser notes that "Resource changes when sliding in a record selector, but react reuses the DOM component". Without the reset the gate would stay open for every record visited after the first edit. Known remaining gap, asserted in the tests rather than left implied: a record holding only a decimal is still marked dirty on open, because the back-fill writes through useFieldParser outside this gate. Nothing is corrupted there - the text is generated from the decimal - but the record is flagged as needing saving. --- .../lib/components/FormPlugins/LatLongUi.tsx | 54 +++++- .../FormPlugins/__tests__/LatLongUi.test.tsx | 168 ++++++++++++++++++ 2 files changed, 221 insertions(+), 1 deletion(-) create mode 100644 specifyweb/frontend/js_src/lib/components/FormPlugins/__tests__/LatLongUi.test.tsx diff --git a/specifyweb/frontend/js_src/lib/components/FormPlugins/LatLongUi.tsx b/specifyweb/frontend/js_src/lib/components/FormPlugins/LatLongUi.tsx index 882adfde6fe..3b9fc3fa210 100644 --- a/specifyweb/frontend/js_src/lib/components/FormPlugins/LatLongUi.tsx +++ b/specifyweb/frontend/js_src/lib/components/FormPlugins/LatLongUi.tsx @@ -38,6 +38,26 @@ function Coordinate({ false ); const isChanging = React.useRef(false); + /** + * Whether the current value arrived from an actual change — the user typing, or + * one of the two resourceOn handlers below reacting to a field being set + * elsewhere — as opposed to simply being read off the resource when the form + * first rendered. Only a real change may write back; see the guard in the + * parsing effect. + */ + const hasValueChanged = React.useRef(false); + /* + * Declared before every other effect so it runs first on a resource swap. + * A record selector slides a NEW resource into this same component instance — + * useFieldParser notes that "Resource changes when sliding in a record + * selector, but react reuses the DOM component". Without this reset the latch + * stays set after any edit and the write-back gate below would stand open for + * every subsequent record, reintroducing the very corruption this guards. + */ + React.useEffect(() => { + hasValueChanged.current = false; + }, [resource, coordinateTextField]); + React.useEffect( () => resourceOn( @@ -49,6 +69,12 @@ function Coordinate({ (resource.get(coordinateTextField) ?? '') === '' && (resource.get(coordinateField) ?? '') !== '' ) + /* + * Deliberately does NOT set hasValueChanged: this handler fires on + * mount (resourceOn(..., true)), so treating it as a change would + * write to a record the curator has only opened. Display updates; + * nothing persists until there is a real edit. + */ updateValue(resource.get(coordinateField)); }, true @@ -65,6 +91,7 @@ function Coordinate({ if (isChanging.current) return; const coordinate = resource.get(coordinateField)?.toString() ?? ''; const parsed = (fieldType === 'Lat' ? Lat : Long).parse(coordinate); + hasValueChanged.current = true; updateValue(parsed?.asFloat() ?? null); }, // Only run this when coordinate field is changed externally @@ -101,6 +128,24 @@ function Coordinate({ : undefined ); + /** + * Opening a record must never modify it. + * + * Everything above this point is display-only — the validation message and + * the formatted "Parsed" column. Everything below writes to the resource and + * marks it dirty, so it may only run in response to an actual user edit. + * + * Without this guard the effect fires on first render and rewrites + * coordinateTextField with the trimmed string. trimLatLong() drops every + * character outside [\s\d"'\-.:ensw°], so a locality stored as "96° 57' O" + * (Spanish Oeste = West, longitude -96.95) is silently rewritten to + * "96° 57' " and +96.95 — the opposite hemisphere — and the evidence that it + * was ever West is destroyed. The verbatim text is the value of record here; + * the decimal is derived from it. That makes this data loss, not a display + * concern, and it happens without the user touching a field. + */ + if (!hasValueChanged.current) return; + isChanging.current = true; /** @@ -145,12 +190,19 @@ function Coordinate({ ]); const isReadOnly = React.useContext(ReadOnlyContext); + const handleValueChange = React.useCallback( + (newValue: string): void => { + hasValueChanged.current = true; + updateValue(newValue); + }, + [updateValue] + ); return ( ); } diff --git a/specifyweb/frontend/js_src/lib/components/FormPlugins/__tests__/LatLongUi.test.tsx b/specifyweb/frontend/js_src/lib/components/FormPlugins/__tests__/LatLongUi.test.tsx new file mode 100644 index 00000000000..5db0d4ee373 --- /dev/null +++ b/specifyweb/frontend/js_src/lib/components/FormPlugins/__tests__/LatLongUi.test.tsx @@ -0,0 +1,168 @@ +/** + * Opening a Locality record must not modify it. + * + * LatLongUi recomputes the decimal coordinate from the verbatim text inside a + * React effect. That effect is keyed on the rendered value, so it fired on the + * FIRST render — before the user had touched anything — and wrote both the + * decimal and the (blackList-trimmed) text back onto the resource. The text + * write was not silent, so simply opening a record marked it dirty; saving it + * for any unrelated reason then persisted the rewrite. + * + * At the California Academy of Sciences this silently moved 2,534 botany + * localities to the opposite hemisphere: a Spanish "96° 57' O" (Oeste = West, + * stored -96.95) became "96° 57' " and +96.95, destroying the evidence that the + * value had ever been West. + * + * These tests pin the invariant: render must be read-only with respect to the + * resource, and a genuine user edit must still update the derived fields. + */ + +import { act, fireEvent, render, waitFor } from '@testing-library/react'; +import React from 'react'; + +import { requireContext } from '../../../tests/helpers'; +import { tables } from '../../DataModel/tables'; +import { LatLongUi } from '../LatLongUi'; + +requireContext(); + +function makeLocality() { + return new tables.Locality.Resource({ + id: 1, + localityname: 'Cerro El Veinte', + lat1text: "17° 33' N", + latitude1: 17.55, + long1text: "96° 57' O", // Oeste = West + longitude1: -96.95, + srclatlongunit: 2, + }); +} + +function makeDecimalOnlyLocality() { + return new tables.Locality.Resource({ + id: 2, + localityname: 'Imported, decimal only', + latitude1: 17.55, + longitude1: -96.95, + }); +} + +describe('LatLongUi does not mutate the resource on render', () => { + /* + * Record-set navigation reuses the component instance — useFieldParser says so + * outright: "Resource changes when sliding in a record selector, but react + * reuses the DOM component". A one-way "has the value changed" ref therefore + * stays latched after any edit, leaving the write-back gate open for every + * record the curator slides to afterwards. That resurrects the corruption in + * the batch-review workflow, where it does the most damage. + */ + test('sliding to another record does not rewrite it, even after an edit', async () => { + const first = makeLocality(); + const { rerender } = render( + + ); + await waitFor(() => expect(first.get('long1text')).toBeDefined()); + + // Simulate a real user edit on the first record. + const input = document.querySelectorAll('input')[1] as HTMLInputElement; + await act(async () => { + fireEvent.change(input, { target: { value: "96° 57' W" } }); + }); + await waitFor(() => expect(first.get('long1text')).toBe("96° 57' W")); + + // Now slide to a different record, as a record set does. + const second = makeLocality(); + await act(async () => { + rerender( + + ); + }); + await waitFor(() => expect(second.get('long1text')).toBeDefined()); + + expect(second.get('long1text')).toBe("96° 57' O"); + expect(Number(second.get('longitude1'))).toBeCloseTo(-96.95, 6); + expect(second.needsSaved).toBe(false); + }); + + /* + * KNOWN REMAINING GAP, deliberately not fixed here. + * + * A record imported with only a decimal (typical of WorkBench / LocalityUpdate) + * still becomes dirty on open: the mount-time back-fill calls updateValue, and + * useFieldParser writes lat1text NON-silently — a path outside this patch's + * gate. Nothing is corrupted (the text is generated from the decimal, and the + * decimal is not rewritten), but the record is flagged as needing saving. + * + * Asserted as-is so the limitation is visible rather than assumed fixed. The + * scope of this patch is therefore "opening cannot CORRUPT a record", not the + * broader "opening cannot touch a record". + */ + test('KNOWN GAP: a decimal-only record is still marked dirty on open', async () => { + const resource = makeDecimalOnlyLocality(); + expect(resource.needsSaved).toBe(false); + await act(async () => { + render( + + ); + }); + await waitFor(() => expect(resource.get('lat1text')).toBeDefined()); + expect(resource.needsSaved).toBe(true); + // The decimal itself is untouched — no corruption, only a dirty flag. + expect(Number(resource.get('latitude1'))).toBeCloseTo(17.55, 6); + }); + + test('merely rendering leaves the verbatim text untouched', async () => { + const resource = makeLocality(); + render( + + ); + + await waitFor(() => expect(resource.get('long1text')).toBeDefined()); + + // The O must survive. Previously this became "96° 57' ". + expect(resource.get('long1text')).toBe("96° 57' O"); + expect(resource.get('lat1text')).toBe("17° 33' N"); + }); + + test('merely rendering leaves the decimal untouched', async () => { + const resource = makeLocality(); + render( + + ); + + await waitFor(() => expect(resource.get('longitude1')).toBeDefined()); + + // Previously flipped to +96.95. + expect(Number(resource.get('longitude1'))).toBeCloseTo(-96.95, 6); + expect(Number(resource.get('latitude1'))).toBeCloseTo(17.55, 6); + }); + + test('merely rendering does not mark the record as needing saving', async () => { + const resource = makeLocality(); + expect(resource.needsSaved).toBe(false); + + render( + + ); + + await waitFor(() => expect(resource.get('long1text')).toBeDefined()); + + // A dirty record is what let an unrelated Save persist the corruption. + expect(resource.needsSaved).toBe(false); + }); +}); From 8485e03f5036180fdaaaa1c358acf8abcc3a5028 Mon Sep 17 00:00:00 2001 From: Joe Russack Date: Tue, 28 Jul 2026 23:24:14 +0000 Subject: [PATCH 2/3] Lint code with ESLint and Prettier Triggered by d22739398252399e69596ee8665c1ced13902a97 on branch refs/heads/issue-cas-latlong-write-on-render --- .../FormPlugins/__tests__/LatLongUi.test.tsx | 21 ++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/specifyweb/frontend/js_src/lib/components/FormPlugins/__tests__/LatLongUi.test.tsx b/specifyweb/frontend/js_src/lib/components/FormPlugins/__tests__/LatLongUi.test.tsx index 5db0d4ee373..1e67556f7e2 100644 --- a/specifyweb/frontend/js_src/lib/components/FormPlugins/__tests__/LatLongUi.test.tsx +++ b/specifyweb/frontend/js_src/lib/components/FormPlugins/__tests__/LatLongUi.test.tsx @@ -59,7 +59,12 @@ describe('LatLongUi does not mutate the resource on render', () => { test('sliding to another record does not rewrite it, even after an edit', async () => { const first = makeLocality(); const { rerender } = render( - + ); await waitFor(() => expect(first.get('long1text')).toBeDefined()); @@ -74,7 +79,12 @@ describe('LatLongUi does not mutate the resource on render', () => { const second = makeLocality(); await act(async () => { rerender( - + ); }); await waitFor(() => expect(second.get('long1text')).toBeDefined()); @@ -102,7 +112,12 @@ describe('LatLongUi does not mutate the resource on render', () => { expect(resource.needsSaved).toBe(false); await act(async () => { render( - + ); }); await waitFor(() => expect(resource.get('lat1text')).toBeDefined()); From 33d438b5e4221f3b5b5dca21b1ad15be88788691 Mon Sep 17 00:00:00 2001 From: Grant Fitzsimmons <37256050+grantfitzsimmons@users.noreply.github.com> Date: Tue, 25 Aug 2026 20:03:54 +0000 Subject: [PATCH 3/3] Lint code with ESLint and Prettier Triggered by 91831172851da4e272eda262508deeb98b5313e9 on branch refs/heads/issue-cas-latlong-write-on-render --- specifyweb/frontend/js_src/jest.config.cjs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/specifyweb/frontend/js_src/jest.config.cjs b/specifyweb/frontend/js_src/jest.config.cjs index eb33b235af8..aaef15405ea 100644 --- a/specifyweb/frontend/js_src/jest.config.cjs +++ b/specifyweb/frontend/js_src/jest.config.cjs @@ -41,12 +41,12 @@ const config = { // An array of glob patterns indicating a set of files for which coverage information should be collected collectCoverageFrom: [ - '**/*.{js,jsx,ts,tsx}', - '!**/*tests*/**', - '!**/*test*', - '!**/localization/**', - '!**/*Route*/**', - '!**/*Route*', + '**/*.{js,jsx,ts,tsx}', + '!**/*tests*/**', + '!**/*test*', + '!**/localization/**', + '!**/*Route*/**', + '!**/*Route*', ], // The directory where Jest should output its coverage files