refactor: consolidate settings-related state

[achou11]

Fixes #978
Closes #1018
Towards #1019

Couldn't figure out a great way of doing this incrementally so apologies in advance for the large scope of this PR. Would generally recommend following the commits to get a somewhat sequential idea of how changes were applied.

Implementation notes:

• Consolidates the following states into a single state:

  • passcode and obscure code
  • language
  • displayed coordinate format and manual entry coordinate format
  • metrics permissions

  As of this writing, the implementation is re-using the persistence key associated with the coordinate format and manual entry coordinate format, meaning that those are the only values that are explicitly migrated/preserved as part of this PR. During my discussion with @ErikSin about migration of the persistence, we decided that at least as part of the initial implementation, migrating previously stored values wasn't a hard requirement given the nature of the app's current usage. There may still be a path where we try to migrate the previous values anyways, but not a focus for now.

• For all language-related changes, please refer to the details written in fix: improve language selection resolution #1011 to understand the changes. I basically ported the changes from there into this PR because an incremental approach was too tricky. There are some small implementation differences (mostly related to naming) but aside from that, what's written in that PR is still very relevant here.

• Not in love with the naming, but made an effort to distinguish between setting state that acts as source of truth and the "resolved" more granular states that should be used within the app. Basically, any settings-related state that may have a default/fallback value when used in the app falls under this classification. Generally, the resolution of these settings happens in the following order:

  1. Check the relevant fields in the settings state. The settings state should always reflect what was explicitly set by the user and should always be updated via explicit interactions. If a field in the settings state is null, it generally means that the user hasn't done anything to explicitly choose a value for that field.
  2. Use a default/fallback value. In the case that the user has not explicitly set a value (or they have explicitly unset it), then it becomes an app-specific decision to decide what to fallback to.

  Using the displayed coordinate format as an example of the above:

  1. The relevant settings state is a valid value (e.g. 'utm', 'dd', 'dms'), use that value.
  2. The relevant settings state is null, so we default to 'utm'.

  The relevant settings with some form of this approach are:

  • displayed coordinate format
  • manual entry coordinate format
  • language (this is more involved since we also account for system preferences as part of the resolution process)
  • metrics permissions

  It is HIGHLY recommended to use the hooks from resolvedSettings/ when reading settings-related values for the above. Reading from the useSettingsState() hook requires more work and is prone to inconsistency since it doesn't handle resolution of values when they are not explicitly set. However, reading the passcode and obscure code from the useSettingsState() directly is fine, as those will never have a fallback value when not set by the user.

• General features that are affected by the changes:

  • app onboarding (metrics permission in privacy section)
  • Passcode setup and usage
  • Language settings
  • Display of coordinates (displayed observations)
  • Inputs provided when manually entering a coordinate (observation creation)

  The language settings is the only feature that should see different behavior as a result of the changes (since it fixes an existing bug). The other features should not exhibit any notable change in behavior.

[achou11]

moved to a more specific file in this directory

[achou11]

moved to coordinateFormat.ts

[achou11]

welcoming input on whether this feels okay in terms of structure. Was thinking about doing some nesting to group related fields together, but that probably satisfies my OCD more than an actual need 😅

[achou11]

wondering if it's really worth using the same key as before as opposed to starting fresh and using a completely new key. The only pre-existing values associated with this storage key are the coordinate format and manual entry coordinate format.

[ErikSin]

@gmaclennan Andrew and I would like you opinion on this. We discussed this on slack, and we disagree with the best approach. So I am going to write down my thoughts, and he is going to further summarize his thoughts and we were hoping to have you help us come to a consensus.

I believe that having a separate default value and persisted state adds unnecessary technical complexity to solve a problem that doesn’t need solving.

Current Approach in This PR

• The setting state has a default value (not persisted).
• When reading the setting state, we check if a persisted value exists.
• If a persisted value does not exist, we return the default value.

Pros

• The default value can change and be reflected in the app.
• This was necessary for language settings, since the default was based on app settings, ensuring updates were reflected.

Cons

• Adds technical complexity and overhead as we are constantly checking for a persisted state and reconciling two sources of truth.

Proposed Alternative

• On initialization, set the persisted value to the default.
• When reading the value, always read from persisted state—no need to check if it exists as we know it is being set on initialization.

Pros

• Technically simpler to maintain—only one source of truth.
• Avoids ongoing logic to reconcile values within the hook.

Cons

• The default value cannot change dynamically.
• Since it’s persisted, we can't distinguish between user-set values and defaults, meaning changes to defaults won’t be reflected unless users manually update their settings.

While dynamic defaults were necessary for language settings, I don’t think it’s worth maintaining this logic for other settings. I dont think default value will change for setting, and even if they do, these values are not detrimental to the user experience.

Following our principles of test ability and maintainability, we should avoid unnecessary complexity. While I see why this approach was introduced for language settings, I don't think it’s needed for other settings.

[ErikSin]

On a side note, I hadn’t seen the language-related changes earlier, but I think the technical overhead of maintaining the default language might be unnecessary. I understand that the goal is for the app language to follow the device language if the user hasn’t explicitly set a preference, and while I agree that’s the ideal behavior, I’m not sure it was worth the added complexity of introducing a default state. That said, since the work is already done, it may not matter at this point—sorry that we didnt do this architecture exploration before hand.

[achou11]

To address some implementation concerns that have been raised:

Why not specify the default values for certain fields when setting up the store state?

Given that the store state can be persisted, the main arguments I will make are:

1. default values should only be applied when reading persistable state
2. stores should never implicitly persist values (default or not)

In the context of the store setup, this is what is meant by using default values:

function createInitialState() {
  return {
    // Use the default value instead of `null` 
    coordinateFormat: 'utm',
    // Other fields...
   }  
}

#978 is a good example of this of why we shouldn't do this. Using a simplified example, this is a sequence that can occur if you made the above change:

1. The app starts. The initial settings state is { coordinateFormat: 'utm', someOtherField: null }, where the 'utm' is the default value we want to use for the coordinate format. Due to how Zustand's persistence middleware works, this is NOT yet persisted into storage.

2. The user performs an action that updates someOtherField via store.setState({ someOtherField: 'foo' }). The settings state is now { coordinateFormat: 'utm', someOtherField: 'foo' }. This DOES get persisted to storage. Now we've committed the default value of coordinateFormat to storage, despite the user performing an action that had nothing to do with it!

3. Turns out that 'utm' is actually not what we want to use as the default value. We update the code such that 'dd' is now the desired default value when the user has not explicitly made a choice.

4. However, the coordinateFormat with value 'utm' is persisted already, so we defer to that value as opposed to considering the new default value, as we do not know if 'utm' was the old default value that was unintentionally stored or if it was an explicit choice that the user made. As a result, the new default value of 'dd' can never be applied unless the storage is cleared.

In this example, we're talking about a setting that is relatively inconsequential in the grand scheme of things. However, there's nothing that prevents this happening for data that has much larger consequences or causes more problematic issues for users.

I would like to effectively communicate that the SettingsState ideally always represents what's been explicitly determined based on user interactions. Happy to better document this with some guidance.

There is an implicit knowledge that one needs to use the more granular hooks instead of useSettingsState() in order to get resolved settings values

Yes, this is not ideal and a fair criticism. I mainly took this approach because it was very easy to write and test, and felt natural to do with zustand. As currently implemented, I don't really have a great suggestion to address this concern other than sufficient documentation, which probably still is prone to misuse.

An alternative approach could be to do the resolution within the useSettingsState() hook, such that now the app can always just use that hook to access resolved values. Rough first idea of an implementation would look kind of like this:

// Actual implementation might look more complicated to appease TS...
function useSettingsState(selector?: <T>(state: SettingsState) => T) {
  let actualSelector 
  
  if (selector) {
    actualSelector = (state: SettingsState) => selector(stateWithDefaults(state))
  } else {
    actualSelector = (state: SettingsState) => stateWithDefaults(state)
  }
  
  const {instance} = useSettingsStoreContext();
  
  return useStore(instance, actualSelector)
}

function stateWithDefaults(state: SetttingsState) {
  return {
    ...state,
    coordinateFormat: state.coordinateFormat === null ? 'utm' : state.coordinateFormat,
    // Other fields with default values...
  }
}

There are some types complications with this as written, but those are potentially addressable. A couple of drawbacks that come to mind:

• we lose the ability to read the settings state and basically know what's actually persisted in storage (assuming that's been enabled). probably not a big issue since we don't really have cases where that's actually useful in the app (i think?). one could work around this by exporting the useSettingsStoreContext() hook, but that's inconsistent with our general approach for zustand-based state.

• it doesn't work well in the case of values whose resolution relies on external factors. The language is a good example of this, where we need to account for system preferences as well. In this case, one could either:

1. update the above implementation to use useLocales() in useSettingsState()

2. have a separate hook that does the full resolution of the language setting (basically what useLanguageTag() does right now)

(1) is not ideal because now we lose the benefit of granular updates to the settings state i.e. with a naive implementation, any changes to the system-preferred locales updates the whole settings state, despite it only being relevant for one field within that state.

(2) is not ideal because the resolution of the field (and the application of the default if needed) is happening elsewhere.

Given a choice, I think the proper technical choice is (2), but then we run into the same issue related to having to know when to use the whole state hook vs a specialized hook. I guess only one exception is better than N exceptions, but it's still an exception nonetheless.

[achou11]

Current Approach in This PR

- The setting state has a default value (not persisted).
- When reading the setting state, we check if a persisted value exists.
- If a persisted value does not exist, we return the default value.

To clarify, certain fields within the settings state will have defaults defined by app/design requirements. When we read the settings state and check each field, if the field does not have a meaningful persisted value (we use null to communicate this), then the relevant hook that processes that field will use the relevant default value.

EDIT: possible that by "default state", you mean "initial state"? if that's the case, this PR is introducing an initial state that is more resilient/reflective to how user settings works from a design perspective.

Adds technical complexity and overhead as we are constantly checking for a persisted state and reconciling two sources of truth.

Not sure what the "constantly" refers to? We read persisted state when the app starts. If you're referring to resolution for certain fields, yes that happens on every update of the store state but that's because settings can/should be un-settable by users throughout the app's lifetime, in order to allow defaults to be applied if necessary.

re: "reconciling two sources of truth", i could be misunderstanding what this refers to, but from my perspective, the store state remains the sole source of truth for settings that are actually defined by the user. We still have to account for the case where they don't specify some settings in order for the app to be usable, which should be done at runtime without implicitly persisting that choice for the user.

Alternatively, if this is referring to the existence of the more granular hooks, nothing stops us from doing something like this in multiple places:

function Foo() {
  const coordinateFormat = useSettingsState(state => state.coordinateFormat || 'utm')
}

function Bar() {
  const coordinateFormat = useSettingsState(state => state.coordinateFormat || 'utm')
}

While this works fine, it's prone to inconsistency (what if you did 'dd' in Bar by accident?). The implementation of the granular hooks is mostly to mitigate this concern, with the overhead of being another hook you have to know about. I find the overhead to be okay because it's easy to make mistakes in the way shown above.

I’m not sure it was worth the added complexity of introducing a default state.

I think this comment is misunderstanding the language issue a bit. The changes to language settings in this PR is NOT introducing a default state - we already had that but the way it is done is flawed and causes user-facing issues. Instead, what this PR is introducing is (more) properly representing when the user has not specified a language to use, and it's updating the calculation of the default value to use more thoroughly.