Greptile Summary

This PR fixes a state-clobbering bug in SettingsViewModel.loadSettings() where a large combine block would reconstruct SettingsState from scratch on every emission, silently resetting four fields — blockUnknownSenders, incomingMessageSizeLimitKb, sortMessagesBySentTime, and tryPropagationOnFail — back to their data-class defaults whenever any upstream flow fired.

The fix follows the well-established pattern already used for ~30 other fields: read _state.value.field inside the combine lambda to carry forward the value managed by its own dedicated collector. It also adds the missing cold-start DataStore collector for tryPropagationOnFail (the other three fields already had collectors; this one did not).

Key changes:

• combine block (SettingsViewModel.kt ~line 406, 443–447): adds _state.value preservation for all four previously-dropped fields, resolving the "briefly flashes then reverts" regression from Messsage Size limit #688
• loadLocationSharingSettings() (~line 1576): adds the tryPropagationOnFailFlow collector so the correct persisted value is loaded on cold start — though tryPropagationOnFail is a delivery setting, not a location-sharing one (see inline comment)
• SettingsViewModelIncomingMessageLimitTest.kt: adds the required mock for tryPropagationOnFailFlow so existing tests continue to pass with the new collector in init {}

Confidence Score: 4/5

Safe to merge — the bug fix is correct and complete; the only remaining issue is a minor organisational concern about where the new collector lives.

The four-field state-preservation fix is correct and matches the established pattern for the rest of the combine block. The previously-flagged cold-start gap for tryPropagationOnFail is now addressed. The sole remaining issue is that the new collector was added to loadLocationSharingSettings() rather than a semantically appropriate home, which is a P2 style concern that does not affect runtime behaviour.

SettingsViewModel.kt lines 1576–1580 and 444/446 — the tryPropagationOnFail collector and its associated combine-block comment both live in location-sharing code.

Important Files Changed

Sequence Diagram

sequenceDiagram
    participant DS as DataStore
    participant SC as Separate Collector<br/>(loadLocationSharingSettings)
    participant CB as combine block<br/>(loadSettings)
    participant ST as _state

    Note over DS,ST: Before fix — race condition clobbers separate collector
    DS->>SC: tryPropagationOnFailFlow emits saved=false
    SC->>ST: _state.update { tryPropagationOnFail = false }
    DS->>CB: any of 15 flows emits
    CB->>ST: SettingsState(..., tryPropagationOnFail = default(true)) ❌

    Note over DS,ST: After fix — combine preserves current state value
    DS->>SC: tryPropagationOnFailFlow emits saved=false
    SC->>ST: _state.update { tryPropagationOnFail = false }
    DS->>CB: any of 15 flows emits
    CB->>ST: SettingsState(..., tryPropagationOnFail = _state.value.tryPropagationOnFail = false) ✅

This is a comment left during a code review.
Path: app/src/main/java/com/lxmf/messenger/viewmodel/SettingsViewModel.kt
Line: 1576-1580

Comment:
**`tryPropagationOnFail` collector misplaced in `loadLocationSharingSettings()`**

The new collector for `tryPropagationOnFail` was added inside `loadLocationSharingSettings()`, but `tryPropagationOnFail` is a message-delivery setting, not a location-sharing one. A future developer looking for where this setting is loaded would not think to look here.

This is consistent with the pre-existing placement of `incomingMessageSizeLimitKb` and `sortMessagesBySentTime` collectors in the same function, but adding a third unrelated setting compounds the confusion. Ideally these three collectors — and the `combine`-block comments "from `loadLocationSharingSettings()`" on lines 444/446 — would move to a dedicated `loadMessageDeliverySettings()` function mirroring the pattern used for `loadPrivacySettings()` and `loadNotificationsSettings()`.

This doesn't affect runtime correctness because `loadLocationSharingSettings()` is always called unconditionally from `init {}`, but it will make the next contributor's grep for where `tryPropagationOnFail` is initialised misleading.

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (2): Last reviewed commit: "fix: load tryPropagationOnFail from Data..." | Re-trigger Greptile}

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!