Do not rewrite Locality coordinate fields when a record is merely opened - #8369
Open
foozleface wants to merge 4 commits into
Open
Do not rewrite Locality coordinate fields when a record is merely opened#8369foozleface wants to merge 4 commits into
foozleface wants to merge 4 commits into
Conversation
…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.
|
Warning One or more dependencies are approaching or past End-of-Life. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughChangesCoordinate write-back behavior
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Triggered by d227393 on branch refs/heads/issue-cas-latlong-write-on-render
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
specifyweb/frontend/js_src/lib/components/FormPlugins/__tests__/LatLongUi.test.tsx (1)
66-71: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the derived coordinate after the edit.
This only verifies the direct text-field update. Use a numerically different longitude (for example,
97° 0' W) and assertlongitude1becomes-97; otherwise a regression that disables edit write-back can still pass.Proposed test adjustment
- fireEvent.change(input, { target: { value: "96° 57' W" } }); + fireEvent.change(input, { target: { value: "97° 0' W" } }); }); - await waitFor(() => expect(first.get('long1text')).toBe("96° 57' W")); + await waitFor(() => { + expect(first.get('long1text')).toBe("97° 0' W"); + expect(Number(first.get('longitude1'))).toBeCloseTo(-97, 6); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specifyweb/frontend/js_src/lib/components/FormPlugins/__tests__/LatLongUi.test.tsx` around lines 66 - 71, Update the test around the first record’s edit and the input change to use a numerically distinct longitude such as 97° 0' W, then also assert that the derived longitude1 value on first becomes -97. Retain the existing long1text assertion so both direct text persistence and coordinate write-back are verified.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@specifyweb/frontend/js_src/lib/components/FormPlugins/LatLongUi.tsx`:
- Around line 72-77: Update the coordinate text-field handler in the LatLongUi
component to ignore only the initial mount callback while setting
hasValueChanged for subsequent change events, allowing the guarded parsing
effect to recompute derived decimal values after external lat1text/long1text
updates. Add test coverage verifying later external text-field changes trigger
the update without treating the initial callback as an edit.
---
Nitpick comments:
In
`@specifyweb/frontend/js_src/lib/components/FormPlugins/__tests__/LatLongUi.test.tsx`:
- Around line 66-71: Update the test around the first record’s edit and the
input change to use a numerically distinct longitude such as 97° 0' W, then also
assert that the derived longitude1 value on first becomes -97. Retain the
existing long1text assertion so both direct text persistence and coordinate
write-back are verified.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fe6a7a1f-bbcc-4a8e-ab37-d4607357ec57
📒 Files selected for processing (2)
specifyweb/frontend/js_src/lib/components/FormPlugins/LatLongUi.tsxspecifyweb/frontend/js_src/lib/components/FormPlugins/__tests__/LatLongUi.test.tsx
github-project-automation
Bot
moved this from 📋Back Log
to Dev Attention Needed
in General Tester Board
This was referenced Aug 3, 2026
grantfitzsimmons
approved these changes
Aug 25, 2026
grantfitzsimmons
left a comment
grantfitzsimmons
left a comment
Member
There was a problem hiding this comment.
I created some test cases:
INSERT INTO locality (
LocalityName,
Latitude1,
Lat1Text,
Longitude1,
Long1Text,
SrcLatLongUnit,
DisciplineID,
TimestampCreated,
TimestampModified
) VALUES
-- 1. Spanish/French West ('O' for Oeste/Ouest) -> High risk of hemisphere flip to positive East
('TEST - Spanish West (O)', 15.0305560000, '15° 01'' 50" N', -90.3372220000, '90° 20'' 14" O', 1, 3, NOW(), NOW()),
-- 2. Portuguese West ('W' or 'O') + South ('S') -> Hemisphere letter stripping
('TEST - Portuguese West (O)', -23.5505200000, '23° 33'' 01" S', -46.6333080000, '46° 37'' 59" O', 1, 3, NOW(), NOW()),
-- 3. German East ('O' for Osten) -> Risk of re-parser treating 'O' as West or noise
('TEST - German East (O)', 52.5200080000, '52° 31'' 12" N', 13.4049540000, '13° 24'' 17" O', 1, 3, NOW(), NOW()),
-- 4. European Decimal Comma in DDM (Degree Decimal Minutes) -> Risk of truncation
('TEST - European Comma DDM', 52.5083330000, '52° 30,500'' N', 13.3833330000, '13° 23,000'' E', 1, 3, NOW(), NOW()),
-- 5. True Unicode Primes (′ = \u2032, ″ = \u2033) vs ASCII (' / ")
('TEST - Unicode Primes', 38.7534280000, '38° 45′ 12.34″ N', -95.2345670000, '95° 14′ 04.44″ W', 1, 3, NOW(), NOW()),
-- 6. Double Single-Quotes ('') transcribed for Seconds
('TEST - Double Single Quotes', 34.0522300000, '34° 03'''' 08" N', -118.2436800000, '118° 14'''' 37" W', 1, 3, NOW(), NOW()),
-- 7. Signed Zero-Degree Latitude (-00°) near Equator -> Stripping negative on 00
('TEST - Signed Zero Equator', -0.2583330000, '-00° 15'' 30"', -78.5833330000, '-78° 35'' 00"', 1, 3, NOW(), NOW()),
-- 8. Signed Zero-Degree Longitude (-000°) near Prime Meridian
('TEST - Signed Zero Meridian', 51.4825780000, '51° 28'' 57" N', -0.0077780000, '-000° 00'' 28"', 1, 3, NOW(), NOW()),
-- 9. Space-Separated DMS (No degree/minute/second symbols)
('TEST - Space Separated DMS', -12.0888890000, '12 05 20.00 S', -77.0500000000, '77 03 00.00 W', 1, 3, NOW(), NOW()),
-- 10. Colon-Delimited DMS Format (HH:MM:SS style)
('TEST - Colon Delimited DMS', 32.7157380000, '32:42:56.66 N', -117.1610840000, '117:09:39.90 W', 1, 3, NOW(), NOW()),
-- 11. Preserved Leading Zeros in Padded DMS Strings
('TEST - Padded Leading Zeros', 8.0672220000, '08° 04'' 02" N', 4.1238890000, '004° 07'' 26" E', 1, 3, NOW(), NOW()),
-- 12. Qualitative/Approximation Prefix in Verbatim Text
('TEST - Approx Prefix ca.', 32.2500000000, 'ca. 32° 15'' N', -110.9500000000, 'ca. 110° 57'' W', 1, 3, NOW(), NOW());On main, I found the following:
- I was prompted to save on Portuguese West, German East, Unicode Primes,
- I could not save due to invalid coords on European Comma DDM or Approx Prefix ca.
- I had no problem with Double Single Quotes, Signed Zero Equator, Signed Zero Meridian, Space Separated DMS, Colon Delimited DMS, or Spanish West.
On issue-cas-latlong-write-on-render, I found the following:
- I had no problem with everything, except I could not save due to invalid coords on Approx Prefix ca.
After your fixes, I verified that I was no longer prompted to save. On main, I was prompted to save, which caused me to update that record inadvertently.
Triggered by 9183117 on branch refs/heads/issue-cas-latlong-write-on-render
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Opening a Locality record currently rewrites its coordinate fields, with no user edit, and marks the record dirty. Saving it afterwards — for any unrelated reason — persists a lossy re-interpretation over the verbatim field.
Mechanism
LatLongUi.tsxre-parses and writes back inside an effect keyed on the rendered value, so it runs on first render:The decimal write is silent; the text write is not.
trimmedValueis a lossy transform of the verbatim transcription, which is the field of record.Observed on a real record
casbotany locality "1 Km a San Miguel Chicaj.", Municipio de Salama, Guatemala. Stored
Long1Text = "90° 20'14\" O",Longitude1 = -90.3372222222. Open it, touch nothing, press Save:ModifiedByAgentbecomes the person who opened it, so afterwards it looks like a deliberate curator edit.Why this is worth fixing on its own
We built four variants of v7.12.0.5 and ran the same browser test against each:
Fixing the parser alone changes nothing, because the write-back strips the offending character and the resulting "clean" text then parses successfully as a wrong value. This change is what actually stops the data loss, which is why it is separated from the parser work in #STACKED.
The change
Gate the write-back on an actual change — the user typing, or either
resourceOnhandler reacting to a field set elsewhere. Display behaviour is unchanged; only persistence is gated.The flag resets on
[resource, coordinateTextField], because a record selector slides a new resource into the same component instance (useFieldParser: "Resource changes when sliding in a record selector, but react reuses the DOM component"). Without the reset the gate would stand open for every record visited after the first edit — this was caught in review and is covered by a test.Known remaining gap
A record holding only a decimal is still marked dirty on open: the back-fill writes through
useFieldParser, outside this gate. Nothing is corrupted — the text is generated from the decimal — but the record is flagged. This is asserted in the tests as aKNOWN GAPrather than left implied, so the scope of this PR is "opening cannot corrupt a record", not the broader "opening cannot touch a record".Tests
LatLongUi.test.tsx— render leaves text and decimal untouched, does not mark the record dirty, and does not rewrite a second record after navigation. Verified to fail with the gate removed.Summary by CodeRabbit