chore: refactor observation fields by ErikSin · Pull Request #1903 · digidem/comapeo-mobile

ErikSin · 2026-05-21T23:13:54Z

closes: #1882

I refactored the Observation Fields to follow our new guidelines.

Previously the ObservationField screen had several nested components. Those nested components had several unnecessary props.

This refactors introduces only one level of nesting, and the nested components have agnostic props instead of passing the entire field object.

I also created several tests. A general integration test that mounts the entire navigation, and tests the observation fields in a normal user interaction. As well as individual tests for each of the smaller components

socket-security · 2026-05-21T23:15:53Z

All alerts resolved. Learn more about Socket for GitHub.

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

View full report

ErikSin · 2026-05-26T18:08:04Z

im not sure why, but the front end integration test is failing on CI, but is not faiing locally. Can you try running the tests locally @cimigree

cimigree · 2026-05-26T18:16:58Z

im not sure why, but the front end integration test is failing on CI, but is not faiing locally. Can you try running the tests locally @cimigree

Yes they pass for me too. I can look into it as I review

ErikSin · 2026-05-26T18:45:19Z

im not sure why, but the front end integration test is failing on CI, but is not faiing locally. Can you try running the tests locally @cimigree

Yes they pass for me too. I can look into it as I review

OK! I think the problem is because we mock/create individual instances of the entire backend in our tests. Since we are doing that, we never install the the backend packages when running the front end CI tests. I was importing the categories for the tests via the backend packages. That is also why it was working locally, because we both have those packages installed locally.

To fix this, i just added the default categories as a dev dep to the front end, and updated the import in the tests

cimigree

Nice refactor!
Applies the guidelines, as you said.
Sub-components now take agnostic props, logic lives at the top level,
<Question> confusion is gone
Lots of great new integration tests.
Cautions
We will need to make sure that if the default-categories update, we update them in our frontend, i.e., keep them in sync manually.
The tests rely on real categories, which is probably fine, but if they change, the test will need to change.

Questions/ Cautions
The Android hardware back button goes back to the Observation screen instead of the previous question. Is that intentional?
Number now stores a float instead of a string. Are you sure that is merited?

Must Change
I am worried about someone entering incomplete info in the number field accidentally. Just a negative sign, or just a point (.) or -0. and it will be not a number. I can't test what actually happens because we don't have any number fields in the default categories, but will it crash the app? It definitely fails an integration test. It can be easily guarded against with isFinite(and check the input).
And then you can add a test to check those inputs.

And other little changes, see below.

cimigree · 2026-05-26T22:14:27Z

+  const field = fields.find(val => val.docId === fieldIds[current - 1]);

  if (!field) {
+    // should throw error here


probably should remove comment and throw error

cimigree · 2026-05-26T22:16:13Z

+        <HeaderText variant="header3">
+          <FormattedFieldProp field={field} propName="label" />
+        </HeaderText>
+        {<HeaderText variant="header5">{field.helperText}</HeaderText>}


Why is there a wrapper around this? Also, field.helpText can be undefined. So then an empty header text renders leaving a big space. See photo
Maybe do this instead?
{field.helperText && <HeaderText variant="header5">{field.helperText}</HeaderText>}

You know what...i didn't realize that these were superflous wrapper. I think they were created in legacy mapeo with an attempt to guarantee a label, when labels were optional. but now labels are mandatory, so i think i can just get rid of the wrapper completely

i removed all the wrappers entirely, which means i had to refactor a couple more pages. They were a remnant of the mapeo and they were really convoluted.

5fb9afb

I also wrote several tests related to showing these details in an observation

cimigree · 2026-05-26T22:25:11Z

+      headerTitle: formatMessage(m.title, {current, total: fieldIds.length}),
      headerLeft: props => (
-        <CustomHeaderLeft headerBackButtonProps={props} onPress={onBackPress} />
+        <CustomHeaderLeft


So now the android back button goes away from the details and back to the Observation screen (as far as I can tell) when it used to do the same thing as the header back button, that is go back one question. Is this your intention?

cimigree · 2026-05-26T22:42:44Z

-              .replace(/[^0-9.-]/g, '') // Allow digits, decimal, and negative sign
-              .replace(/(?!^)-/g, '') // Remove any minus sign that is not at the start
-              .replace(/(\..*?)\./g, '$1'), // Remove additional decimal points
+            parseFloat(


parseFloat('-'), parseFloat('.'), and parseFloat('-0.') all return NaN It would be good to put something here so incomplete input doesn't save NaN into the observation tags

e95471d and 33d8f7d

Wow number are annoying. I added so many tests to catch the edge cases! But also the tests made it really easy to find edge cases and determine if it breaks things!

cimigree · 2026-05-26T22:45:03Z

-        testID="OBS.details-inp"
-        value={typeof value === 'string' ? value : ''}
+        testID="OBS.number-inp"
+        value={typeof tagValue === 'number' ? String(tagValue) : ''}


If someone is editing an existing observation that has a number field, it will show a blank field because this value used to be saved as a string.

see comment below!

cimigree · 2026-05-26T22:48:14Z

-              .replace(/[^0-9.-]/g, '') // Allow digits, decimal, and negative sign
-              .replace(/(?!^)-/g, '') // Remove any minus sign that is not at the start
-              .replace(/(\..*?)\./g, '$1'), // Remove additional decimal points
+            parseFloat(


This field used to be stored as a string but now it will be stored as a float. That does make sense, but it is a change. Is that for sure intentional?

handled now. if the value is a string (that can be parsed as a number) it will show the value. and then if they update the value, that string will be updated to a number. 33d8f7d

cimigree · 2026-05-26T22:54:57Z

+    const details = await screen.findByText('Details');
+    await user.press(details);
+
+    // through manually clicking through the app, I know that this should be a text input


This code comment should be a little less conversational for posterity...Maybe something like the category type Animal's first field is a text input (animal-notes).

cimigree · 2026-05-26T22:58:16Z

+  fireEvent.changeText(screen.getByDisplayValue(''), '1.2.3');
+
+  expect(updateTag).toHaveBeenCalledWith(1.23);
+});


If you had added fireEvent.changeText(screen.getByDisplayValue(''), '-');
fireEvent.changeText(screen.getByDisplayValue(''), '.');
It would fail... Once you fix that, you can add that test!

cimigree

Wow this is quite a refactor!
With the new config I was able to test numbers!
One thing that worked was using the old version of a number (that was a string) and then switching branches to this new version of number (that is a number). I couldn't even tell it was processing. All worked as expected.

BUT,
I am having 2 issues with numbers.

is that I cannot enter negative numbers using the keyboard given. Not sure if that is a problem. I can paste them in... Not sure if that is expected. So regex makes sense to keep but wondering about use case and needing negative numbers. It may be related to the numeric keyboard type. But I am not sure.
If I have a number in place and then I erase it so that there is no number (because I want to have "no answer") then it doesn't work. The first digit from the previous entry is kept and cannot be cleared. I can type 0 as a work around but I am not sure how intuitive that is and may be misleading or not what a user wants. See video.

Screen_Recording_20260528_180406_CoMapeo.Dev.mp4

Also, my local jest tests are failing.

 FAIL  src/frontend/screens/ComapeoSettings/index.test.tsx (5.355 s)
  ● CoMapeo Settings Screen › opens drawer when header button is pressed

    TypeError: (0 , _reactNative.renderAsync) is not a function

      49 |     onTeardown.push(appProviders.teardown);

And

FAIL  src/frontend/screens/ObservationFields/index.test.tsx
 ● Test suite failed to run

   Cannot find module '@comapeo/default-categories/package.json' from 'src/frontend/screens/ObservationFields/index.test.tsx'

     4 |
     5 | const DEFAULT_CONFIG_PATH = path.join(
   > 6 |   path.dirname(require.resolve('@comapeo/default-categories/package.json')),
       |                        ^
     7 |   'dist/comapeo-default-categories.comapeocat',
     8 | );
     9 |

     at Resolver._throwModNotFoundError (node_modules/jest-resolve/build/resolver.js:427:11)
     at Object.resolve (src/frontend/screens/ObservationFields/index.test.tsx:6:24)

And a bunch of tests on Browserstack are failing. Not sure if that is flakiness or the tests because it is Observations that are failing.

awana-lockfile-bot · 2026-06-11T17:52:21Z

`package-lock.json` changes

Click to toggle table visibility

Name	Status	Previous	Current
`@testing-library/react-native`		13.2.0	13.3.3

socket-security · 2026-06-11T17:53:21Z

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff	Package	Supply Chain Security	Vulnerability	Quality	Maintenance	License
	npm/@testing-library/react-native@13.2.0 ⏵ 13.3.3	⁺¹			⁺¹

View full report

ErikSin · 2026-06-11T18:10:38Z

@cimigree this number issue seems device specific. I have updated the pr to be similar to the previous implementation. Can you try using numbers on develop branch, and then try using numbers on this branch. If they both have the same problem, then lets just merge this pr and open an issue

cimigree

It actually works fine on develop and on Samsung. Seems like all is fixed. I tested all manually though I see there are so many new and thorough tests. Way to go!

One non blocking comment!

cimigree · 2026-06-17T16:30:20Z

+            .replace(/[^0-9.-]/g, '') // Allow digits, decimal, and negative sign
+            .replace(/(?!^)-/g, '') // Remove any minus sign that is not at the start
+            .replace(/(\..*?)\./g, '$1') // Remove additional decimal points
+            // .replace(/^(-?)\./, '$10.') // Prefix bare decimal with 0


I think this line that you commented out was the one that was breaking the functionality on the Samsung. Can you just delete it instead of leaving it commented out?

ErikSin added 18 commits May 20, 2026 13:32

chore: create new observation field screen

7337f1d

chore: use old field types

9b47302

chore: update select one

c946700

chore: update select one

c45cbb2

chore: select multiple

ae34cd6

chore: number and text area

19ff222

chore: update tag value

97c1fb1

chore: update tyings

9ec978d

chore: update actions row

0fe3b6a

chore: create test file

42ef4eb

Merge branch 'develop' into chore-refactor-observation-fields

3f8c6db

chore: make render navigation async

351233e

chore: add config

ad59a5c

chore: create tests

16aa60a

chore: write tests for each component

2d8fc51

chore: update test id

9195c66

chore: create tests

2e544b3

chore: change text button color

c8806a6

ErikSin requested a review from cimigree May 21, 2026 23:14

chore: resolve categories from front end

724e165

cimigree requested changes May 26, 2026

View reviewed changes

ErikSin added 5 commits May 27, 2026 15:55

Merge branch 'develop' into chore-refactor-observation-fields

34e0a1e

chore: handle edge cases and update tests

33d8f7d

chore: remove unncessary labels

a35523d

chore: handle -0 and -0{digit}

e95471d

chore: remove unnecessary wrapper

5fb9afb

ErikSin added 5 commits May 28, 2026 11:50

chore: write tests

84c2234

chore: integration tests

c5fe4b4

chore: fix back actions

09e26cb

chore: error if no field

27e33ce

chore: update comment

5ffb9c9

ErikSin requested a review from cimigree May 28, 2026 19:48

fix: properly navigate away from fields

2172909

cimigree requested changes May 28, 2026

View reviewed changes

ErikSin added 10 commits June 3, 2026 14:07

Merge branch 'develop' into chore-refactor-observation-fields

81c0a9a

chore: allow number to be undefined

6eb7d3c

chore: allow number to be null

f333f4b

chore: better integration tests

d29b9f2

chore: add view

94eacbc

chore: add accessibility state to select multiple

592cab0

chore: update tests

7a07394

chore: update tests

b40779f

Merge branch 'develop' into chore-refactor-observation-fields

6500308

chore: update sanitation

f573cd3

chore: remove unused message

99aa0b7

ErikSin requested a review from cimigree June 11, 2026 18:09

cimigree approved these changes Jun 17, 2026

View reviewed changes

chore: remove comment

3c08212

ErikSin merged commit 3576633 into develop Jun 17, 2026
10 of 12 checks passed

ErikSin deleted the chore-refactor-observation-fields branch June 17, 2026 17:36

Uh oh!

Conversation

ErikSin commented May 21, 2026

Uh oh!

socket-security Bot commented May 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ErikSin commented May 26, 2026

Uh oh!

cimigree commented May 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ErikSin commented May 26, 2026

Uh oh!

cimigree left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cimigree left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

awana-lockfile-bot Bot commented Jun 11, 2026

package-lock.json changes

Uh oh!

socket-security Bot commented Jun 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ErikSin commented Jun 11, 2026

Uh oh!

cimigree left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

socket-security Bot commented May 21, 2026 •

edited

Loading

cimigree commented May 26, 2026 •

edited

Loading

cimigree left a comment •

edited

Loading

`package-lock.json` changes

socket-security Bot commented Jun 11, 2026 •

edited

Loading