fix(persistence): restore non-dock app state on relaunch (#94)#95
Open
TigerInYourDream wants to merge 4 commits intomainfrom
Open
fix(persistence): restore non-dock app state on relaunch (#94)#95TigerInYourDream wants to merge 4 commits intomainfrom
TigerInYourDream wants to merge 4 commits intomainfrom
Conversation
handle_load_app_state previously gated RestoreAppStateFromPersistentState behind a non-empty dock-state check, which silently dropped bot_settings, app_language, and translation on every mobile relaunch (mobile has no dock). Desktop masked the bug because dock state is almost always non-empty after first run. Unconditionally dispatch the restore action when load_app_state succeeds; the restore match arm in app.rs already handles empty dock correctly via full AppState replacement and unconditional LoadDockFromAppState dispatch. Add a serde round-trip unit test as a regression guard so a future #[serde(skip)] on bot_settings is caught at `cargo test` time instead of at Android runtime. Spec: specs/task-fix-mobile-appservice-persistence.spec.md Plan: docs/superpowers/plans/2026-04-14-fix-mobile-appservice-persistence.md Issue: issues/009-mobile-appservice-binding-not-persisted.md Closes #94
Codex review caught that the initial fix dispatched RestoreAppStateFromPersistentState on every Ok(load_app_state) — but load_app_state returns Ok(AppState::default()) when no file exists. That path silently overwrote the in-memory default with another default and contradicted the spec's fresh-install scenario which requires no restore dispatch. Tighten the gate to a content-aware check: restore only when the loaded state has any meaningfully non-default field (dock, bot_settings, app_language, translation). Fresh-install / no-file path is a true no-op again. - src/sliding_sync.rs: add should_restore_loaded_app_state(); gate the dispatch on it; log a "nothing to restore" line for default state - src/sliding_sync.rs (tests): add positive (#94 case) and negative (fresh install) guards in matrix_request_tests - specs/task-fix-mobile-appservice-persistence.spec.md: update Decisions to reflect content-aware gate; spec scenarios were already correct - issues/009-mobile-appservice-binding-not-persisted.md: tone down "lifecycle 8/8 pass" — the manual_test_* selectors run 0 tests under cargo test even though structural verdict is pass Verification (in robrix2-review-94 worktree): cargo build -p robrix → ok cargo test should_restore_loaded_app_state → ok cargo test test_app_state_roundtrip_preserves_bot_settings_with_empty_dock → ok cargo test -p robrix --lib → 253 passed / 4 failed (pre-existing)
The original issue #94 body and my initial spec / plan / issue-doc all referenced `rs.robius.robrix` as the Android package ID. During manual verification this was found wrong — the actual package is `dev.makepad.robrix`, which is why `adb shell am force-stop rs.robius.robrix` had no effect. Fix the three places where the stale name appears: - specs/task-fix-mobile-appservice-persistence.spec.md (scenario 2 step) - issues/009-mobile-appservice-binding-not-persisted.md (reproduction step 6) - docs/superpowers/plans/2026-04-14-fix-mobile-appservice-persistence.md (Step 4.3 manual test instructions) No code change. Android manual test path now works end-to-end with the corrected package name.
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
1 participant
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.
Summary
handle_load_app_stategatedRestoreAppStateFromPersistentStatebehind a non-empty dock-state check.bot_settings,app_language,translation) out of the restore path. Desktop masked this because dock state is almost always non-empty after first run; mobile has no dock, so the bug was 100% reproducible.AppStatehas any meaningfully non-default persisted field, and keep the fresh-install / no-file path as a true no-op.Closes #94.
Changes
src/sliding_sync.rs::handle_load_app_state— newshould_restore_loaded_app_state()helper; dispatchRestoreAppStateFromPersistentStateonly when the loaded state carries real content. Fresh-install path logs"nothing to restore"and is a genuine no-op.src/sliding_sync.rs(mod matrix_request_tests) — two guards: positive case (bot_settings set + empty dock must restore) and negative case (pure default must not).src/app.rs(mod tests) — serde round-trip regression guard (test_app_state_roundtrip_preserves_bot_settings_with_empty_dock) that pins the persistence contract so a future#[serde(skip)]onbot_settingsis caught atcargo testtime.specs/task-fix-mobile-appservice-persistence.spec.md— new agent-spec Task Contract (Chinese, quality 93%, lifecycle command passes). Covers 7 scenarios including fresh-install, corrupt file, skip-restore marker, stale room binding prune.docs/superpowers/plans/2026-04-14-fix-mobile-appservice-persistence.md— implementation plan.issues/009-mobile-appservice-binding-not-persisted.md— local issue doc with Fix Applied section.Commit history (3 commits)
b7ede0bf fix(persistence): restore non-dock app state on relaunch (#94)— initial fix + spec + plan + regression teste8f6bfc3 fix(persistence): preserve fresh-install no-op path (#94 review fix)— Codex review caught thatOk(load_app_state)on a fresh install returnsOk(AppState::default()), which the initial fix wrongly restored; tightened to content-aware gate + added 2 guard tests642cb94e docs(#94): correct Android package name to dev.makepad.robrix— docs had stalers.robius.robrix; corrected after manual verificationTest plan
cargo build -p robrix— cleancargo test -p robrix --lib test_app_state_roundtrip_preserves_bot_settings_with_empty_dock— PASScargo test -p robrix --lib should_restore_loaded_app_state— PASS (positive)cargo test -p robrix --lib test_should_not_restore_loaded_default_app_state— PASS (negative)cargo test -p robrix --lib— 253 passed / 4 failed (same 4 pre-existing failures unrelated to this change; verified viagit stash)agent-spec lint specs/task-fix-mobile-appservice-persistence.spec.md --min-score 0.7— quality 93%adb shell am force-stop dev.makepad.robrix+ relaunch → fields restored, Check Now Reachable