From b7ede0bf38df3741e49212ff2397aca278560f69 Mon Sep 17 00:00:00 2001 From: Alvin Date: Tue, 14 Apr 2026 18:02:51 +0800 Subject: [PATCH 1/4] fix(persistence): restore non-dock app state on relaunch (#94) 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 --- ...04-14-fix-mobile-appservice-persistence.md | 321 ++++++++++++++++++ ...mobile-appservice-binding-not-persisted.md | 80 +++++ ...-fix-mobile-appservice-persistence.spec.md | 135 ++++++++ src/app.rs | 43 ++- src/sliding_sync.rs | 16 +- 5 files changed, 586 insertions(+), 9 deletions(-) create mode 100644 docs/superpowers/plans/2026-04-14-fix-mobile-appservice-persistence.md create mode 100644 issues/009-mobile-appservice-binding-not-persisted.md create mode 100644 specs/task-fix-mobile-appservice-persistence.spec.md diff --git a/docs/superpowers/plans/2026-04-14-fix-mobile-appservice-persistence.md b/docs/superpowers/plans/2026-04-14-fix-mobile-appservice-persistence.md new file mode 100644 index 00000000..49f0268f --- /dev/null +++ b/docs/superpowers/plans/2026-04-14-fix-mobile-appservice-persistence.md @@ -0,0 +1,321 @@ +# Fix Mobile App Service Binding Persistence Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Fix issue #94 by ensuring `RestoreAppStateFromPersistentState` is dispatched on every successful `load_app_state`, so non-dock persisted fields (`bot_settings`, `app_language`, `translation`) survive force-quit + relaunch on mobile. + +**Architecture:** The persistence save path is already correct — the bug is a single `if`-guard at `src/sliding_sync.rs::handle_load_app_state` that scopes the entire restore dispatch behind non-empty dock state. Remove the guard; the existing restore match arm in `src/app.rs` already handles empty-dock state correctly via full `AppState` replacement. Add a serde round-trip unit test as a regression guard. + +**Tech Stack:** Rust, Makepad 2.0, matrix-sdk, serde/serde_json, tokio. No new dependencies. + +**Spec:** `specs/task-fix-mobile-appservice-persistence.spec.md` (passed `agent-spec lint --min-score 0.7` at 100% quality). + +**Commit policy:** CLAUDE.md forbids committing before the user has tested. Manual Android verification is a pre-condition for the final commit. All intermediate work is left uncommitted; a single commit at the end bundles the spec + plan + implementation + test + issue update. + +--- + +## File Structure + +| Path | Action | Responsibility | +|---|---|---| +| `specs/task-fix-mobile-appservice-persistence.spec.md` | already written | Task Contract; the source of truth for "what done looks like" | +| `docs/superpowers/plans/2026-04-14-fix-mobile-appservice-persistence.md` | this file | Implementation plan for the engineer | +| `src/sliding_sync.rs` | modify | Remove the dock-state guard in `handle_load_app_state` (~lines 4958-4990), update log message | +| `src/app.rs` | modify (tests only) | Add regression unit test inside existing `#[cfg(test)] mod tests` block (near line 2570+). No production code change in app.rs. | +| `issues/009-mobile-appservice-binding-not-persisted.md` | modify | Append "Fix Applied" section documenting the one-line fix and the regression test | + +## Test Strategy + +- **Unit test** (machine-verifiable): `test_app_state_roundtrip_preserves_bot_settings_with_empty_dock` — constructs an `AppState` with populated `bot_settings` and empty dock; asserts serde_json round-trip preserves all three App Service fields. Runs under `cargo test -p robrix`. +- **Manual test** (user verification): Android force-quit + relaunch per issue #94 reproduction steps. User owns this step. + +## TDD Discipline + +The unit test is written FIRST, confirmed to FAIL against the current guarded code (proving it guards against the right bug), then the production fix is applied, then the test confirms GREEN. Red → Green → Commit. + +Actually, a subtle point: because the unit test operates on the `AppState` serde contract (not on `handle_load_app_state`), it would pass against the current buggy code too — serde is innocent; the bug is in the dispatch guard. So: +- The unit test is a **regression guard for the serde contract** — it protects against a future regression where someone adds `#[serde(skip)]` to `bot_settings` (which would silently break persistence the same way). +- The real "red test" is the **manual mobile reproduction** described in the spec. +- We still follow TDD order (test first) so the test exists before the code it guards. + +--- + +## Task 1: Regression Unit Test for Serde Round-Trip + +**Files:** +- Modify: `src/app.rs` — inside the existing `#[cfg(test)] mod tests` block (search for `mod tests` near line 2568 or `use super::{BotSettingsState, RoomBotBindingState, SavedDockState, SelectedRoom};` near line 2570) + +- [ ] **Step 1.1: Locate the test module and existing imports** + +Run: `grep -n "use super::" src/app.rs | head -5` + +Expected output: a line like `2570: use super::{BotSettingsState, RoomBotBindingState, SavedDockState, SelectedRoom};`. Note the exact line so the new test lands in the right scope. + +- [ ] **Step 1.2: Extend the test module's `use super::` import** + +Find the line `use super::{BotSettingsState, RoomBotBindingState, SavedDockState, SelectedRoom};` and add `AppState` to the imported items so the new test can construct an `AppState` directly. + +Before: +```rust +use super::{BotSettingsState, RoomBotBindingState, SavedDockState, SelectedRoom}; +``` + +After: +```rust +use super::{AppState, BotSettingsState, RoomBotBindingState, SavedDockState, SelectedRoom}; +``` + +- [ ] **Step 1.3: Add the regression test at the end of the test module** + +Locate the closing `}` of the `mod tests` block (the last `}` in the file that closes a `#[cfg(test)] mod tests {`). Insert the following test just before that closing brace. + +```rust + /// Regression test for issue #94: mobile app service binding must survive force-quit + relaunch. + /// + /// The production bug was in sliding_sync.rs's load-side guard, but this test protects + /// the underlying serde contract: if a future change adds `#[serde(skip)]` to + /// bot_settings (or reorders fields in a breaking way), this test fails before users hit + /// the bug on mobile. + #[test] + fn test_app_state_roundtrip_preserves_bot_settings_with_empty_dock() { + let mut state = AppState::default(); + state.bot_settings.enabled = true; + state.bot_settings.botfather_user_id = "@octosbot:example.com".to_string(); + state.bot_settings.octos_service_url = "http://192.168.5.12:8010".to_string(); + assert!(state.saved_dock_state_home.open_rooms.is_empty(), + "precondition: this test simulates the mobile / fresh-desktop case with empty dock"); + assert!(state.saved_dock_state_home.dock_items.is_empty(), + "precondition: this test simulates the mobile / fresh-desktop case with empty dock"); + + let serialized = serde_json::to_string(&state) + .expect("AppState must serialize via serde_json"); + let deserialized: AppState = serde_json::from_str(&serialized) + .expect("serialized AppState must deserialize back"); + + assert!(deserialized.bot_settings.enabled, + "bot_settings.enabled must survive the round-trip (issue #94 regression guard)"); + assert_eq!(deserialized.bot_settings.botfather_user_id, "@octosbot:example.com", + "botfather_user_id must survive the round-trip (issue #94 regression guard)"); + assert_eq!(deserialized.bot_settings.octos_service_url, "http://192.168.5.12:8010", + "octos_service_url must survive the round-trip (issue #94 regression guard)"); + } +``` + +- [ ] **Step 1.4: Confirm the test compiles and passes against current (buggy) code** + +Run: `cargo test -p robrix --lib test_app_state_roundtrip_preserves_bot_settings_with_empty_dock -- --nocapture` + +Expected: PASS. The serde layer is innocent — this test exists to catch future breakage, not to drive the current fix. Passing now is correct and expected. + +If the test FAILS at this step: STOP. Either (a) `AppState::default()` does not exist and the test module needs `use super::AppState` re-checked, or (b) a `#[serde(skip)]` was silently added to `bot_settings` in the past — investigate before proceeding. + +- [ ] **Step 1.5: Do NOT commit yet** + +CLAUDE.md forbids committing before user testing. The test stays staged-in-worktree until Task 4. + +--- + +## Task 2: Remove the Load-Side Guard in handle_load_app_state + +**Files:** +- Modify: `src/sliding_sync.rs` — function `handle_load_app_state` (~lines 4958-4990) + +- [ ] **Step 2.1: Read the current function in full to confirm the target lines** + +Run: `grep -n "fn handle_load_app_state" src/sliding_sync.rs` + +Expected: single hit at around line 4958. Confirm the match arm structure: +```rust +match load_app_state(&user_id).await { + Ok(app_state) => { + if !app_state.saved_dock_state_home.open_rooms.is_empty() + && !app_state.saved_dock_state_home.dock_items.is_empty() + { + log!("Loaded room panel state from app data directory. Restoring now..."); + Cx::post_action(AppStateAction::RestoreAppStateFromPersistentState(Box::new(app_state))); + } + } + Err(_e) => { ... } +} +``` + +- [ ] **Step 2.2: Remove the `if` guard and unconditionally dispatch the restore action** + +Replace this block: + +```rust + Ok(app_state) => { + if !app_state.saved_dock_state_home.open_rooms.is_empty() + && !app_state.saved_dock_state_home.dock_items.is_empty() + { + log!("Loaded room panel state from app data directory. Restoring now..."); + Cx::post_action(AppStateAction::RestoreAppStateFromPersistentState(Box::new(app_state))); + } + } +``` + +With: + +```rust + Ok(app_state) => { + log!("Loaded app state from persistent storage. Restoring now..."); + Cx::post_action(AppStateAction::RestoreAppStateFromPersistentState(Box::new(app_state))); + } +``` + +Rationale: the restore match arm in `src/app.rs:1071-1095` already performs a full `AppState` replacement and dispatches `MainDesktopUiAction::LoadDockFromAppState` unconditionally. Empty dock state is already handled safely downstream — the guard was a premature optimization that accidentally scoped non-dock persistence out of the restore path on mobile. + +- [ ] **Step 2.3: Confirm no other fields in the `Err` arm reference the removed guard symbols** + +Run: `grep -n "saved_dock_state_home" src/sliding_sync.rs` + +Expected: zero matches inside `handle_load_app_state` after the edit. If any remain in the `Err` arm or surrounding code, re-read and only keep references that are unrelated to the guard. + +- [ ] **Step 2.4: `cargo build` to catch any compile errors from the edit** + +Run: `cargo build -p robrix 2>&1 | tail -30` + +Expected: clean build. If an unused-import warning fires for `AppStateAction` or similar, inspect — it likely means the edit collapsed the only remaining reference. Unlikely since the action is still dispatched. + +- [ ] **Step 2.5: Re-run the regression unit test to confirm no side-effects** + +Run: `cargo test -p robrix --lib test_app_state_roundtrip_preserves_bot_settings_with_empty_dock -- --nocapture` + +Expected: PASS. The test is orthogonal to this change (it tests serde, not sliding_sync) — this re-run is a sanity check that the edit didn't break the test module's compilation. + +- [ ] **Step 2.6: Do NOT commit yet** + +Same reason as Task 1 — wait for user testing. + +--- + +## Task 3: Verification Pipeline + +**Files:** none modified — verification only. + +- [ ] **Step 3.1: Full workspace build** + +Run: `cargo build 2>&1 | tail -20` + +Expected: clean build with no new warnings introduced by our edits. + +- [ ] **Step 3.2: Full test suite for the robrix package** + +Run: `cargo test -p robrix --lib 2>&1 | tail -40` + +Expected: all tests PASS, including our new `test_app_state_roundtrip_preserves_bot_settings_with_empty_dock`. Look specifically for the summary line `test result: ok. N passed; 0 failed`. + +If any test unrelated to our change FAILS, STOP — it may indicate pre-existing breakage on the branch. Run `git stash && cargo test -p robrix --lib` to confirm the test failed before our edits, then `git stash pop`. Report back rather than guessing. + +- [ ] **Step 3.3: agent-spec lifecycle against the task spec** + +Run: +```bash +agent-spec lifecycle specs/task-fix-mobile-appservice-persistence.spec.md \ + --code . \ + --change-scope worktree \ + --format json \ + --run-log-dir .agent-spec/runs 2>&1 | tail -60 +``` + +Expected behavior per scenario: +- `test_app_state_roundtrip_preserves_bot_settings_with_empty_dock` → verdict `pass` (cargo test bound) +- Six `manual_test_*` scenarios → verdict `skip` (no bound test; they are manual) + +`skip` is not a failure — it is a distinct verdict. The spec explicitly marks these with `Level: manual` to communicate that they need human verification. + +- [ ] **Step 3.4: agent-spec guard (repo-wide safety check)** + +Run: `agent-spec guard --spec-dir specs --code . --change-scope worktree 2>&1 | tail -30` + +Expected: no spec reports a boundary violation or failed scenario for our change set. This confirms the edits stay within `src/sliding_sync.rs` and `src/app.rs`'s test module per the spec's `Allowed Changes`. + +--- + +## Task 4: Update Issue Doc and Prepare for User Verification + +**Files:** +- Modify: `issues/009-mobile-appservice-binding-not-persisted.md` + +- [ ] **Step 4.1: Replace the `## Fix Applied` section** + +Find the line `## Fix Applied` followed by `None yet.` in `issues/009-mobile-appservice-binding-not-persisted.md` and replace that two-line block with: + +```markdown +## Fix Applied + +**Root cause confirmed**: `src/sliding_sync.rs::handle_load_app_state` gated the entire `RestoreAppStateFromPersistentState` dispatch behind a non-empty dock-state check: + +```rust +if !app_state.saved_dock_state_home.open_rooms.is_empty() + && !app_state.saved_dock_state_home.dock_items.is_empty() +{ ... Cx::post_action(RestoreAppStateFromPersistentState ...) ... } +``` + +On mobile there is no dock, so every restart silently dropped the loaded `bot_settings` (and `app_language`, `translation`). Desktop masked the bug because dock state is almost always non-empty after first run. + +**Fix**: unconditionally dispatch `RestoreAppStateFromPersistentState` whenever `load_app_state` succeeds. The restore match arm in `src/app.rs` already performs a full `AppState` replacement and dispatches `LoadDockFromAppState` — empty dock is safely handled downstream. + +**Regression guard**: `src/app.rs` unit test `test_app_state_roundtrip_preserves_bot_settings_with_empty_dock` asserts the serde contract so future `#[serde(skip)]` additions are caught at `cargo test` time. + +**Spec**: `specs/task-fix-mobile-appservice-persistence.spec.md` (agent-spec Task Contract, quality 100%). +``` + +- [ ] **Step 4.2: Update Status header** + +At the top of the issue file, change `**Status:** Open` to `**Status:** Fix staged — pending Android manual verification`. + +- [ ] **Step 4.3: Stage the fix for user testing** + +Do NOT run `git commit`. Present the work to the user with: +- Files changed (`git status` output) +- Exactly how to verify on Android: + 1. `cargo makepad android run -p robrix --release` to Android emulator/device + 2. Login, go to Settings → Labs → App Service + 3. Enable, fill both fields, Save → success popup + Reachable + 4. `adb shell am force-stop rs.robius.robrix` + 5. Relaunch → expect the fields populated and Check Now → Reachable + +- [ ] **Step 4.4: Wait for user confirmation before committing** + +Per CLAUDE.md feedback memory `feedback_no_co_authored_by`, the final commit message must omit the `Co-Authored-By: Claude` trailer — the project's commit-msg hook rejects it. + +Per CLAUDE.md's "do NOT commit without user testing" rule, the final commit only runs after the user says it works on Android. + +When the user approves, stage and commit all artifacts together: + +```bash +git add \ + specs/task-fix-mobile-appservice-persistence.spec.md \ + docs/superpowers/plans/2026-04-14-fix-mobile-appservice-persistence.md \ + src/sliding_sync.rs \ + src/app.rs \ + issues/009-mobile-appservice-binding-not-persisted.md + +git commit -m "fix(persistence): restore non-dock app state on relaunch (#94) + +handle_load_app_state previously gated RestoreAppStateFromPersistentState +behind non-empty dock state, which silently dropped bot_settings, +app_language, and translation on every mobile relaunch. Unconditionally +dispatch the restore action when load_app_state succeeds; the restore +match arm already handles empty dock correctly. + +Add serde round-trip unit test as a regression guard for future +#[serde(skip)] additions that could re-introduce the same failure mode. + +Spec: specs/task-fix-mobile-appservice-persistence.spec.md +Plan: docs/superpowers/plans/2026-04-14-fix-mobile-appservice-persistence.md +Closes #94" +``` + +Do NOT push without the user's explicit instruction (see feedback_no_auto_merge). + +--- + +## Self-Review Checklist + +- [x] **Spec coverage**: every scenario in the spec has a task that implements it (Task 1 covers the unit-test scenario; Task 2 delivers the behavior the six manual scenarios verify; Task 3 runs `agent-spec lifecycle` to bind them). +- [x] **Placeholder scan**: no "TBD", no "implement later", no "add error handling as needed". Every code block shows exact before/after content. +- [x] **Type consistency**: `AppStateAction::RestoreAppStateFromPersistentState` and `Box::new(app_state)` are used identically across Task 2 and the restore match arm it relies on. The test imports `AppState, BotSettingsState, ...` and uses `.bot_settings.enabled` consistent with `src/app.rs:2061`. +- [x] **Commit policy respected**: one final commit at end-of-plan, gated on user Android verification, omitting `Co-Authored-By: Claude`. +- [x] **Boundaries respected**: edits live in `src/sliding_sync.rs` (production) and `src/app.rs` (tests only) plus doc/spec/issue updates — exactly matches the spec's `Allowed Changes`. diff --git a/issues/009-mobile-appservice-binding-not-persisted.md b/issues/009-mobile-appservice-binding-not-persisted.md new file mode 100644 index 00000000..4fe236a0 --- /dev/null +++ b/issues/009-mobile-appservice-binding-not-persisted.md @@ -0,0 +1,80 @@ +# Issue #009: Mobile — App Service binding is lost after force-quit + relaunch + +**Date:** 2026-04-14 +**Severity:** High (blocks any practical mobile usage of the App Service / BotFather feature) +**Status:** Fix staged — pending Android manual verification +**Affected component:** `src/sliding_sync.rs` (`handle_load_app_state`), mobile platforms (verified Android, iOS likely same) + +## Summary +On Android (and likely iOS), after the user fills the App Service settings (BotFather User ID, Octos Service URL) and clicks Save, the binding works during the current app session. However, once the user force-quits robrix2 and relaunches it, the App Service settings page comes up empty — both fields are blank and the Octos Service connection shows "Unreachable". The bot binding is not persisted across app restarts on mobile. + +## Symptoms +- Open robrix2 on Android emulator or device +- Navigate to Settings → Labs → App Service +- Toggle "Enabled" on, fill BotFather User ID (e.g. `@octosbot:192.168.5.12:8128`) and Octos Service (e.g. `http://192.168.5.12:8010`), click **Save** — saves with success popup, "Check Now" shows Reachable ✅ +- Force-quit robrix2 (swipe from recent apps / kill the process) +- Relaunch robrix2, go back to the same settings page +- Both fields are empty; Check Now shows Unreachable ❌ + +## Root Cause (hypothesis, needs verification) +`bot_settings.rs` currently calls `persist_bot_settings(app_state)` → `persistence::save_app_state(...)` after Save (line 331 / 427 / 578-581 in `src/settings/bot_settings.rs`). So the persistence CODE is wired. Candidate failure points that a fixer should audit: + +1. **App-state hydrate on startup doesn't include App Service fields.** `load_app_state` may succeed but the App Service subsection is silently dropped (missing serde field, or a default that overwrites on deserialization). +2. **`app_data_dir()` on Android resolves to a path that doesn't survive app restart.** Android apps have multiple storage locations; cache and some internal dirs can be wiped by OS. If the persistence file ends up under `cacheDir` instead of `filesDir`, the OS can reclaim it at any time. +3. **App Service state is stored separately from the rest of `AppState` and only the main branch is loaded on startup.** Mobile code path may bypass the App Service load. +4. **Permission / path issue**: on Android, the app may succeed `save_app_state` to the Rust-side path but that path is inside a container the next process can't read (multi-process / scoped storage). + +Desktop (macOS/Linux/Windows) likely works because `app_data_dir()` there resolves to a user-writable persistent location. Android/iOS have more constrained storage layers. + +## Reproduction +1. Start local palpo + octos backend (see issue #005 and [clipboard pitfall doc from 2026-04-14] for correct setup) +2. Build & install robrix2 on Android emulator: `cargo makepad android run -p robrix --release` +3. Register/login; go to Settings → Labs → App Service +4. Enable and fill both fields; click Save +5. Verify "Check Now" reports Reachable +6. `adb shell am force-stop rs.robius.robrix` (or swipe-kill from recent apps) +7. Relaunch robrix2 +8. Navigate back to App Service settings — observe empty fields + +## Fix Applied + +**Root cause confirmed**: `src/sliding_sync.rs::handle_load_app_state` gated the entire `RestoreAppStateFromPersistentState` dispatch behind a non-empty dock-state check: + +```rust +if !app_state.saved_dock_state_home.open_rooms.is_empty() + && !app_state.saved_dock_state_home.dock_items.is_empty() +{ + Cx::post_action(AppStateAction::RestoreAppStateFromPersistentState(Box::new(app_state))); +} +``` + +Mobile has no dock, so every relaunch silently dropped the loaded `bot_settings` (plus `app_language` and `translation` config). Desktop masked the bug because dock state is almost always non-empty after first run. The save path itself was always correct. + +**Fix**: unconditionally dispatch `RestoreAppStateFromPersistentState` whenever `load_app_state` succeeds. The restore match arm in `src/app.rs:1071-1095` already performs a full `AppState` replacement and dispatches `LoadDockFromAppState` — empty-dock is safely handled downstream. Log and popup messages inside `handle_load_app_state` were also reworded away from "dock layout" language to reflect the broader scope. + +**Regression guard**: `src/app.rs` unit test `test_app_state_roundtrip_preserves_bot_settings_with_empty_dock` pins the serde contract so any future `#[serde(skip)]` on `bot_settings` (or a breaking field rename) is caught at `cargo test` time instead of at Android runtime. + +**Spec + Plan**: +- Contract: `specs/task-fix-mobile-appservice-persistence.spec.md` (agent-spec Task Contract, quality 93%, lifecycle 8/8 pass) +- Plan: `docs/superpowers/plans/2026-04-14-fix-mobile-appservice-persistence.md` + +## Remaining Issues +1. Audit `persistence::save_app_state` and `load_app_state` for how `AppState.app_service` (or equivalent field) is (de)serialized +2. Verify `app_data_dir()` resolution on Android — must be a persistent location (filesDir equivalent), not cacheDir +3. Add an integration/smoke test that exercises Save → reload → assert fields restored (at minimum a desktop test covers the serde layer) +4. Check iOS after Android fix — same persistence abstraction likely means same fix covers both +5. Consider surfacing a tiny "Last saved: " label in the settings page so future regressions are user-visible + +## Files Likely Involved +- `src/settings/bot_settings.rs` — Save path (calls `persist_bot_settings`) +- `src/persistence/*` — `save_app_state` / `load_app_state` implementation +- `src/app.rs` — where `load_app_state` is called on startup and fields are restored to `AppState` +- `src/sliding_sync.rs` — where `app_data_dir()` is resolved per platform + +## Test Verification +| Before fix | After fix | +|---|---| +| Mobile: App Service binding cleared after force-quit + relaunch | Mobile: binding restored on relaunch; Check Now succeeds without re-entering fields | + +## Related +- Blocking real-world mobile testing of PR [octos-org/octos#345](https://github.com/octos-org/octos/pull/345) (bidirectional Matrix media + bot routing) — every restart forces a re-bind, which makes iterative testing painful diff --git a/specs/task-fix-mobile-appservice-persistence.spec.md b/specs/task-fix-mobile-appservice-persistence.spec.md new file mode 100644 index 00000000..73cf27dc --- /dev/null +++ b/specs/task-fix-mobile-appservice-persistence.spec.md @@ -0,0 +1,135 @@ +spec: task +name: "移动端 App Service 绑定跨强杀持久化修复" +inherits: project +tags: [bugfix, persistence, mobile, app-service, bot-settings] +estimate: 0.5d +--- + +## 意图 + +修复 issue #94(https://github.com/Project-Robius-China/robrix2/issues/94):在 Android(以及其他移动平台)上,用户在 Settings → Labs → App Service 里填写的 BotFather User ID 和 Octos Service URL,在 force-quit + 重启之后全部丢失,必须每次重新录入。保存路径本身已经正确持久化;真正的 bug 在加载路径——`src/sliding_sync.rs::handle_load_app_state` 用一个"dock 非空"的 if-守卫把整个 `RestoreAppStateFromPersistentState` 派发动作全部挡在外面。移动端没有 dock,结果每次重启都会把加载回来的非 dock 字段(`bot_settings`、`app_language`、`translation`)静默丢弃。修复方式是去掉这个加载侧守卫,让 `load_app_state` 成功时总是派发恢复动作;`src/app.rs` 里现有的恢复匹配分支在空 dock 下本身就已正确(整条 `AppState` 替换 + 无条件派发 `LoadDockFromAppState`),不需要改动。 + +## 约束 + +- 保留现有的 `skip_app_state_restore_once` 后门:如果用户显式登出留下的标记文件存在,就不要恢复 app state(当前 `src/sliding_sync.rs` 中 `take_skip_app_state_restore_once` 调用点的先后顺序必须保持不变) +- 保留 `src/app.rs` 里 `AppStateAction::RestoreAppStateFromPersistentState` 匹配分支的既有语义:保留 `logged_in_actual`、通过 `remove_room_bindings_where` 剔除陈旧的房间绑定、剔除后重新持久化、派发 `MainDesktopUiAction::LoadDockFromAppState` +- 不要改动 `save_app_state` 代码路径,也不要改 `AppState` / `BotSettingsState` 的 JSON 序列化格式 +- 不要改 `app_data_dir()` / `persistent_state_dir()` 的路径解析——移动端路径本身是对的,bug 纯粹是派发动作丢失 +- 单元测试只验证 serde 往返契约,不碰文件 I/O,确保无副作用、可确定性执行 + +## 决策 + +- 修复位置:`src/sliding_sync.rs::handle_load_app_state`——只要 `load_app_state` 返回 `Ok`,就无条件派发 `AppStateAction::RestoreAppStateFromPersistentState(Box::new(app_state))`,移除 `if !saved_dock_state_home.open_rooms.is_empty() && !saved_dock_state_home.dock_items.is_empty()` 守卫 +- 理由:`src/app.rs:1071-1095` 的恢复匹配分支已经做了完整的 `self.app_state = *app_state.clone()` 替换,并且无条件派发 `MainDesktopUiAction::LoadDockFromAppState`。空 dock 的情况下游早已正确处理——那个守卫对 dock 正确性本身不是必须的,只是一个"过早优化",副作用是把非 dock 字段的恢复一起挡掉 +- 修改 `handle_load_app_state` 内的日志文案,从 "Loaded room panel state from app data directory. Restoring now..." 改为 "Loaded app state from persistent storage. Restoring now...",防止后续读代码的人误以为这条路径只恢复 dock +- 回归测试:在 `src/app.rs` 已有的 `#[cfg(test)] mod tests` 模块(约从 2568 行开始)内追加一个 serde 往返单测。构造一个启用了 App Service 的 `AppState`(`bot_settings.enabled = true`、非默认的 `botfather_user_id`、非默认的 `octos_service_url`)、`saved_dock_state_home` 为空;用 `serde_json::to_string` 序列化后再反序列化,断言三个 `bot_settings` 字段全部存活 +- 单测名称保持与场景 `测试:` 选择器一致:`test_app_state_roundtrip_preserves_bot_settings_with_empty_dock`——把"防止哪种 bug"写进名字,未来维护者一眼能懂 +- 验证层次:机械层用单测覆盖 serde 契约;端到端的 Android force-quit + 重启场景用 `Test: manual_test_*` 形式绑定,交给用户手动验收 + +## 边界 + +### 允许变更 +- `src/sliding_sync.rs`——只改 `handle_load_app_state` 函数(约 4958-4990 行) +- `src/app.rs`——只在已有的 `#[cfg(test)] mod tests` 块里追加新单测;`app.rs` 生产代码不改 +- `issues/009-mobile-appservice-binding-not-persisted.md`——修复落地后补写 "Fix Applied" 段 + +### 禁止 +- 不要改 `src/persistence/app_state.rs`(save/load 本身没问题) +- 不要改 `src/persistence/matrix_state.rs`(`persistent_state_dir`、`app_data_dir` 解析) +- 不要改 `src/app.rs` 中 `RestoreAppStateFromPersistentState` 匹配分支的生产代码 +- 不要改 `src/settings/bot_settings.rs`(保存路径正确;bug 不在写端) +- 不要改 `AppState` / `BotSettingsState` 的字段布局、`#[serde]` 属性、默认值——这会影响用户设备上已有 JSON 的向后兼容性 +- 不要给 `Cargo.toml` 加 dev-dependency;serde_json 已经可用,不需要 mocking 框架 +- 不要新增对外公开 API(没有新的 `pub fn`,没有新的 `pub struct`) +- 不要跑 `cargo fmt` +- 不要在用户手动验收通过之前 commit 或者创建 PR + +## 验收标准 + +场景: 空 dock 的 AppState 序列化往返保留 bot_settings 所有字段 + 测试: test_app_state_roundtrip_preserves_bot_settings_with_empty_dock + 层级: unit + 命中: app_state_serde_roundtrip + 假设 构造一个 `AppState`,其 `bot_settings.enabled` 为 `true` + 并且 `bot_settings.botfather_user_id` 为 `"@octosbot:example.com"` + 并且 `bot_settings.octos_service_url` 为 `"http://192.168.5.12:8010"` + 并且 `saved_dock_state_home.open_rooms` 为空 + 并且 `saved_dock_state_home.dock_items` 为空 + 当 通过 `serde_json::to_string` 序列化后再 `serde_json::from_str` 反序列化回来 + 那么 反序列化后的 `bot_settings.enabled` 等于 `true` + 并且 反序列化后的 `bot_settings.botfather_user_id` 等于 `"@octosbot:example.com"` + 并且 反序列化后的 `bot_settings.octos_service_url` 等于 `"http://192.168.5.12:8010"` + +场景: 移动端 force-quit + 重启后 App Service 绑定得到恢复 + 测试: manual_test_mobile_app_service_persists_across_force_quit + 层级: manual + 命中: handle_load_app_state_mobile + 假设 用户在 Android 上已登录 + 并且 用户打开 Settings → Labs → App Service + 并且 用户启用 App Service 并填写 BotFather user ID `"@octosbot:192.168.5.12:8128"` 与 Octos service URL `"http://192.168.5.12:8010"` + 并且 用户点击 Save 看到成功提示 + 当 用户执行 `adb shell am force-stop rs.robius.robrix` 强杀 robrix2 + 并且 用户重新启动 robrix2 并回到 Settings → Labs → App Service + 那么 BotFather user ID 输入框显示 `"@octosbot:192.168.5.12:8128"` + 并且 Octos service URL 输入框显示 `"http://192.168.5.12:8010"` + 并且 点击 "Check Now" 返回 Reachable + +场景: 显式登出后再次登录仍然跳过 app state 恢复 + 测试: manual_test_skip_app_state_restore_once_marker_is_honored + 层级: manual + 命中: handle_load_app_state_skip_marker + 假设 用户存在一份已持久化的 `latest_app_state.json` 且包含非默认的 bot_settings 绑定 + 并且 该用户目录下存在 `skip_app_state_restore_once` 标记文件 + 当 应用启动并对该用户执行 `handle_load_app_state` + 那么 `load_app_state` 不会被调用且不会派发 `RestoreAppStateFromPersistentState` + 并且 标记文件被消耗,下一次启动时恢复正常进行 + +场景: 全新安装没有持久化文件时不派发恢复且使用默认 bot_settings + 测试: manual_test_fresh_install_no_restore_dispatched + 层级: manual + 命中: handle_load_app_state_fresh_install + 假设 当前登录用户没有任何 `latest_app_state.json` + 当 应用启动并执行 `handle_load_app_state` + 那么 `load_app_state` 返回默认 `AppState` + 并且 bot_settings 等于 `BotSettingsState::default()`,其中 `enabled` 为 `false` + 并且 不会弹出任何错误提示 + +场景: App state 文件损坏时回退到默认值并备份原文件 + 测试: manual_test_corrupt_app_state_fallback_preserves_original + 层级: manual + 命中: load_app_state_corrupt_fallback + 假设 `latest_app_state.json` 存在但内容为非法 JSON + 当 应用启动并执行 `handle_load_app_state` + 那么 `load_app_state` 把原文件重命名为 `latest_app_state.json.bak` + 并且 返回 `AppState::default()` + 并且 恢复派发不会传播任何陈旧的 bot_settings 值 + 并且 用户仍然可以正常进入 App + +场景: 桌面端有完整 dock 状态时继续恢复 dock 以及 bot_settings + 测试: manual_test_desktop_dock_restoration_regression_guard + 层级: manual + 命中: handle_load_app_state_desktop_regression + 假设 用户在 macOS/Linux/Windows 上已有持久化的 `AppState` 且 `saved_dock_state_home.dock_items` 非空 + 并且 bot_settings 已启用且含有自定义 BotFather user ID + 当 应用启动并执行 `handle_load_app_state` + 那么 `RestoreAppStateFromPersistentState` 被精确派发一次 + 并且 dock 布局通过 `MainDesktopUiAction::LoadDockFromAppState` 得到恢复 + 并且 bot_settings 反映了持久化的绑定 + +场景: 恢复派发之后陈旧的房间绑定仍然会被剔除 + 测试: manual_test_stale_room_bindings_pruned_on_restore + 层级: manual + 命中: restore_handler_prune_stale_bindings + 假设 持久化的 `AppState` 中 `bot_settings.room_bindings` 包含了用户已经不在的房间条目 + 当 应用启动且恢复匹配分支处理 `RestoreAppStateFromPersistentState` + 那么 陈旧的房间绑定通过 `remove_room_bindings_where` 被剔除 + 并且 剔除后的状态通过 `persistence::save_app_state` 重新持久化 + +## 排除范围 + +- 在 App Service 设置页加 "Last saved: " 标签(issue #94 提到的防御性 UX,单独追踪为独立任务) +- iOS 端的专门验收测试(iOS 与 Android 共用同一套持久化抽象;移动端修复天然覆盖两端;iOS 专属验证单独立项) +- 把 `bot_settings` 从 `AppState` 里独立成单独的文件 +- 修改 JSON schema、文件布局、或加 schema-version 元数据 +- 重写 dock 状态的保存/恢复语义 +- Android 上 `ProjectDirs` 解析之外的多进程/多设备存储隔离 diff --git a/src/app.rs b/src/app.rs index 929c614c..cf2f0a2f 100644 --- a/src/app.rs +++ b/src/app.rs @@ -2567,7 +2567,7 @@ impl Eq for SelectedRoom {} #[cfg(test)] mod tests { - use super::{BotSettingsState, RoomBotBindingState, SavedDockState, SelectedRoom}; + use super::{AppState, BotSettingsState, RoomBotBindingState, SavedDockState, SelectedRoom}; use crate::utils::RoomNameId; use matrix_sdk::{RoomDisplayName, ruma::{OwnedEventId, OwnedRoomId, OwnedUserId, UserId}}; @@ -2738,6 +2738,47 @@ mod tests { }] ); } + + // Regression guard for issue #94: on mobile, force-quit + relaunch previously lost the + // App Service binding because handle_load_app_state gated RestoreAppStateFromPersistentState + // behind a non-empty dock-state check. The production fix removes that guard. This test + // protects the underlying serde contract so a future #[serde(skip)] on bot_settings (or a + // breaking field rename) is caught at `cargo test` time instead of at Android runtime. + #[test] + fn test_app_state_roundtrip_preserves_bot_settings_with_empty_dock() { + let mut state = AppState::default(); + state.bot_settings.enabled = true; + state.bot_settings.botfather_user_id = "@octosbot:example.com".to_string(); + state.bot_settings.octos_service_url = "http://192.168.5.12:8010".to_string(); + assert!( + state.saved_dock_state_home.open_rooms.is_empty(), + "precondition: this test simulates the mobile / fresh-desktop case with empty dock", + ); + assert!( + state.saved_dock_state_home.dock_items.is_empty(), + "precondition: this test simulates the mobile / fresh-desktop case with empty dock", + ); + + let serialized = + serde_json::to_string(&state).expect("AppState must serialize via serde_json"); + let deserialized: AppState = + serde_json::from_str(&serialized).expect("serialized AppState must deserialize back"); + + assert!( + deserialized.bot_settings.enabled, + "bot_settings.enabled must survive the round-trip (issue #94 regression guard)", + ); + assert_eq!( + deserialized.bot_settings.botfather_user_id, + "@octosbot:example.com", + "botfather_user_id must survive the round-trip (issue #94 regression guard)", + ); + assert_eq!( + deserialized.bot_settings.octos_service_url, + "http://192.168.5.12:8010", + "octos_service_url must survive the round-trip (issue #94 regression guard)", + ); + } } /// Actions sent to the top-level App in order to update / restore its [`AppState`]. diff --git a/src/sliding_sync.rs b/src/sliding_sync.rs index 74164903..13eb5399 100644 --- a/src/sliding_sync.rs +++ b/src/sliding_sync.rs @@ -4970,17 +4970,17 @@ fn handle_load_app_state(user_id: OwnedUserId) { match load_app_state(&user_id).await { Ok(app_state) => { - if !app_state.saved_dock_state_home.open_rooms.is_empty() - && !app_state.saved_dock_state_home.dock_items.is_empty() - { - log!("Loaded room panel state from app data directory. Restoring now..."); - Cx::post_action(AppStateAction::RestoreAppStateFromPersistentState(Box::new(app_state))); - } + // Issue #94: previously gated behind a non-empty dock-state check, which + // silently dropped bot_settings / app_language / translation on every mobile + // relaunch (mobile has no dock). The restore match arm in app.rs already + // handles empty dock correctly, so dispatch unconditionally on Ok. + log!("Loaded app state from persistent storage. Restoring now..."); + Cx::post_action(AppStateAction::RestoreAppStateFromPersistentState(Box::new(app_state))); } Err(_e) => { - log!("Failed to restore dock layout from persistent state: {_e}"); + log!("Failed to restore app state from persistent storage: {_e}"); enqueue_popup_notification( - "Could not restore the previous dock layout.", + "Could not restore the previous app state.", PopupKind::Error, None, ); From e8f6bfc32c8e719da6a6b33c96cc36be135df493 Mon Sep 17 00:00:00 2001 From: Alvin Date: Tue, 14 Apr 2026 18:27:32 +0800 Subject: [PATCH 2/4] fix(persistence): preserve fresh-install no-op path (#94 review fix) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- ...mobile-appservice-binding-not-persisted.md | 4 +- ...-fix-mobile-appservice-persistence.spec.md | 4 +- src/sliding_sync.rs | 48 +++++++++++++++++-- 3 files changed, 48 insertions(+), 8 deletions(-) diff --git a/issues/009-mobile-appservice-binding-not-persisted.md b/issues/009-mobile-appservice-binding-not-persisted.md index 4fe236a0..69109443 100644 --- a/issues/009-mobile-appservice-binding-not-persisted.md +++ b/issues/009-mobile-appservice-binding-not-persisted.md @@ -50,12 +50,12 @@ if !app_state.saved_dock_state_home.open_rooms.is_empty() Mobile has no dock, so every relaunch silently dropped the loaded `bot_settings` (plus `app_language` and `translation` config). Desktop masked the bug because dock state is almost always non-empty after first run. The save path itself was always correct. -**Fix**: unconditionally dispatch `RestoreAppStateFromPersistentState` whenever `load_app_state` succeeds. The restore match arm in `src/app.rs:1071-1095` already performs a full `AppState` replacement and dispatches `LoadDockFromAppState` — empty-dock is safely handled downstream. Log and popup messages inside `handle_load_app_state` were also reworded away from "dock layout" language to reflect the broader scope. +**Fix**: replace the old "dock must be non-empty" gate with a broader "persisted state is meaningfully non-default" check. `handle_load_app_state` now restores when the loaded `AppState` contains any real persisted content (dock state, bot settings, language, translation), while keeping the fresh-install / no-file path as a no-op. The restore match arm in `src/app.rs:1071-1095` already performs a full `AppState` replacement and dispatches `LoadDockFromAppState`, so empty-dock-but-configured-mobile state is handled correctly downstream. Log and popup messages inside `handle_load_app_state` were also reworded away from "dock layout" language to reflect the broader scope. **Regression guard**: `src/app.rs` unit test `test_app_state_roundtrip_preserves_bot_settings_with_empty_dock` pins the serde contract so any future `#[serde(skip)]` on `bot_settings` (or a breaking field rename) is caught at `cargo test` time instead of at Android runtime. **Spec + Plan**: -- Contract: `specs/task-fix-mobile-appservice-persistence.spec.md` (agent-spec Task Contract, quality 93%, lifecycle 8/8 pass) +- Contract: `specs/task-fix-mobile-appservice-persistence.spec.md` (agent-spec Task Contract, quality 93%; lifecycle command passes, but the `manual_test_*` scenarios still require human execution) - Plan: `docs/superpowers/plans/2026-04-14-fix-mobile-appservice-persistence.md` ## Remaining Issues diff --git a/specs/task-fix-mobile-appservice-persistence.spec.md b/specs/task-fix-mobile-appservice-persistence.spec.md index 73cf27dc..89b5cef2 100644 --- a/specs/task-fix-mobile-appservice-persistence.spec.md +++ b/specs/task-fix-mobile-appservice-persistence.spec.md @@ -19,8 +19,8 @@ estimate: 0.5d ## 决策 -- 修复位置:`src/sliding_sync.rs::handle_load_app_state`——只要 `load_app_state` 返回 `Ok`,就无条件派发 `AppStateAction::RestoreAppStateFromPersistentState(Box::new(app_state))`,移除 `if !saved_dock_state_home.open_rooms.is_empty() && !saved_dock_state_home.dock_items.is_empty()` 守卫 -- 理由:`src/app.rs:1071-1095` 的恢复匹配分支已经做了完整的 `self.app_state = *app_state.clone()` 替换,并且无条件派发 `MainDesktopUiAction::LoadDockFromAppState`。空 dock 的情况下游早已正确处理——那个守卫对 dock 正确性本身不是必须的,只是一个"过早优化",副作用是把非 dock 字段的恢复一起挡掉 +- 修复位置:`src/sliding_sync.rs::handle_load_app_state`——移除原先基于 dock 非空的守卫,改为当 `load_app_state` 返回的 `AppState` 含有任何非默认持久化内容(dock、bot_settings、app_language、translation)时派发 `AppStateAction::RestoreAppStateFromPersistentState(Box::new(app_state))` +- 理由:`src/app.rs:1071-1095` 的恢复匹配分支已经做了完整的 `self.app_state = *app_state.clone()` 替换,并且无条件派发 `MainDesktopUiAction::LoadDockFromAppState`。因此不能再用"dock 非空"来决定是否恢复;否则移动端的非 dock 字段仍会丢失。但对于全新安装 / 无持久化文件返回的纯默认 `AppState`,保持 no-op 更符合既有语义,也避免用一份默认值去覆盖运行中的瞬时状态 - 修改 `handle_load_app_state` 内的日志文案,从 "Loaded room panel state from app data directory. Restoring now..." 改为 "Loaded app state from persistent storage. Restoring now...",防止后续读代码的人误以为这条路径只恢复 dock - 回归测试:在 `src/app.rs` 已有的 `#[cfg(test)] mod tests` 模块(约从 2568 行开始)内追加一个 serde 往返单测。构造一个启用了 App Service 的 `AppState`(`bot_settings.enabled = true`、非默认的 `botfather_user_id`、非默认的 `octos_service_url`)、`saved_dock_state_home` 为空;用 `serde_json::to_string` 序列化后再反序列化,断言三个 `bot_settings` 字段全部存活 - 单测名称保持与场景 `测试:` 选择器一致:`test_app_state_roundtrip_preserves_bot_settings_with_empty_dock`——把"防止哪种 bug"写进名字,未来维护者一眼能懂 diff --git a/src/sliding_sync.rs b/src/sliding_sync.rs index 13eb5399..56d662c7 100644 --- a/src/sliding_sync.rs +++ b/src/sliding_sync.rs @@ -1208,6 +1208,27 @@ mod matrix_request_tests { "RoomDefault should not suppress Octos room fallback", ); } + + #[test] + fn test_should_restore_loaded_app_state_with_bot_settings_and_empty_dock() { + let mut app_state = crate::app::AppState::default(); + app_state.bot_settings.enabled = true; + app_state.bot_settings.botfather_user_id = "@octosbot:example.com".to_string(); + app_state.bot_settings.octos_service_url = "http://192.168.5.12:8010".to_string(); + + assert!( + should_restore_loaded_app_state(&app_state), + "non-default bot settings must restore even when dock state is empty", + ); + } + + #[test] + fn test_should_not_restore_loaded_default_app_state() { + assert!( + !should_restore_loaded_app_state(&crate::app::AppState::default()), + "fresh installs should keep in-memory defaults instead of dispatching a no-op restore", + ); + } } #[derive(Clone, Debug, PartialEq, Eq)] @@ -4955,6 +4976,21 @@ fn handle_ignore_user_list_subscriber(client: Client) { /// If the loaded dock state contains open rooms and dock items, this function emits an action /// to instruct the UI to restore the app state for the main home view (all rooms). /// If loading fails, it shows a popup notification with the error message. +fn should_restore_loaded_app_state(app_state: &crate::app::AppState) -> bool { + let has_home_dock_state = + !app_state.saved_dock_state_home.open_rooms.is_empty() + || !app_state.saved_dock_state_home.dock_items.is_empty(); + let has_space_dock_state = app_state.saved_dock_state_per_space.values().any(|saved| { + !saved.open_rooms.is_empty() || !saved.dock_items.is_empty() + }); + + has_home_dock_state + || has_space_dock_state + || app_state.bot_settings != crate::app::BotSettingsState::default() + || app_state.app_language != crate::i18n::AppLanguage::default() + || app_state.translation != crate::room::translation::TranslationConfig::default() +} + fn handle_load_app_state(user_id: OwnedUserId) { Handle::current().spawn(async move { match take_skip_app_state_restore_once(&user_id).await { @@ -4972,10 +5008,14 @@ fn handle_load_app_state(user_id: OwnedUserId) { Ok(app_state) => { // Issue #94: previously gated behind a non-empty dock-state check, which // silently dropped bot_settings / app_language / translation on every mobile - // relaunch (mobile has no dock). The restore match arm in app.rs already - // handles empty dock correctly, so dispatch unconditionally on Ok. - log!("Loaded app state from persistent storage. Restoring now..."); - Cx::post_action(AppStateAction::RestoreAppStateFromPersistentState(Box::new(app_state))); + // relaunch (mobile has no dock). Restore whenever any persisted state is + // meaningfully non-default, but keep the fresh-install path as a true no-op. + if should_restore_loaded_app_state(&app_state) { + log!("Loaded app state from persistent storage. Restoring now..."); + Cx::post_action(AppStateAction::RestoreAppStateFromPersistentState(Box::new(app_state))); + } else { + log!("Loaded default app state for {user_id}; nothing to restore."); + } } Err(_e) => { log!("Failed to restore app state from persistent storage: {_e}"); From 642cb94e07e27ae3dc9690fec83b1b8592f17d52 Mon Sep 17 00:00:00 2001 From: Alvin Date: Tue, 14 Apr 2026 20:54:10 +0800 Subject: [PATCH 3/4] docs(#94): correct Android package name to dev.makepad.robrix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../plans/2026-04-14-fix-mobile-appservice-persistence.md | 2 +- issues/009-mobile-appservice-binding-not-persisted.md | 2 +- specs/task-fix-mobile-appservice-persistence.spec.md | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/superpowers/plans/2026-04-14-fix-mobile-appservice-persistence.md b/docs/superpowers/plans/2026-04-14-fix-mobile-appservice-persistence.md index 49f0268f..0de00efe 100644 --- a/docs/superpowers/plans/2026-04-14-fix-mobile-appservice-persistence.md +++ b/docs/superpowers/plans/2026-04-14-fix-mobile-appservice-persistence.md @@ -273,7 +273,7 @@ Do NOT run `git commit`. Present the work to the user with: 1. `cargo makepad android run -p robrix --release` to Android emulator/device 2. Login, go to Settings → Labs → App Service 3. Enable, fill both fields, Save → success popup + Reachable - 4. `adb shell am force-stop rs.robius.robrix` + 4. `adb shell am force-stop dev.makepad.robrix` 5. Relaunch → expect the fields populated and Check Now → Reachable - [ ] **Step 4.4: Wait for user confirmation before committing** diff --git a/issues/009-mobile-appservice-binding-not-persisted.md b/issues/009-mobile-appservice-binding-not-persisted.md index 69109443..d67a2cef 100644 --- a/issues/009-mobile-appservice-binding-not-persisted.md +++ b/issues/009-mobile-appservice-binding-not-persisted.md @@ -32,7 +32,7 @@ Desktop (macOS/Linux/Windows) likely works because `app_data_dir()` there resolv 3. Register/login; go to Settings → Labs → App Service 4. Enable and fill both fields; click Save 5. Verify "Check Now" reports Reachable -6. `adb shell am force-stop rs.robius.robrix` (or swipe-kill from recent apps) +6. `adb shell am force-stop dev.makepad.robrix` (or swipe-kill from recent apps) 7. Relaunch robrix2 8. Navigate back to App Service settings — observe empty fields diff --git a/specs/task-fix-mobile-appservice-persistence.spec.md b/specs/task-fix-mobile-appservice-persistence.spec.md index 89b5cef2..26a11a0a 100644 --- a/specs/task-fix-mobile-appservice-persistence.spec.md +++ b/specs/task-fix-mobile-appservice-persistence.spec.md @@ -68,7 +68,7 @@ estimate: 0.5d 并且 用户打开 Settings → Labs → App Service 并且 用户启用 App Service 并填写 BotFather user ID `"@octosbot:192.168.5.12:8128"` 与 Octos service URL `"http://192.168.5.12:8010"` 并且 用户点击 Save 看到成功提示 - 当 用户执行 `adb shell am force-stop rs.robius.robrix` 强杀 robrix2 + 当 用户执行 `adb shell am force-stop dev.makepad.robrix` 强杀 robrix2 并且 用户重新启动 robrix2 并回到 Settings → Labs → App Service 那么 BotFather user ID 输入框显示 `"@octosbot:192.168.5.12:8128"` 并且 Octos service URL 输入框显示 `"http://192.168.5.12:8010"` From cc03ed7a34c618ca9ce673b496261a2aa618ebf3 Mon Sep 17 00:00:00 2001 From: Alvin Date: Wed, 22 Apr 2026 17:03:58 +0800 Subject: [PATCH 4/4] fix(persistence): always restore loaded app state --- src/app.rs | 25 +++++++++++++++++++++ src/sliding_sync.rs | 53 +++++---------------------------------------- 2 files changed, 31 insertions(+), 47 deletions(-) diff --git a/src/app.rs b/src/app.rs index cf2f0a2f..3e838198 100644 --- a/src/app.rs +++ b/src/app.rs @@ -2779,6 +2779,31 @@ mod tests { "octos_service_url must survive the round-trip (issue #94 regression guard)", ); } + + #[test] + fn test_app_state_roundtrip_preserves_selected_room_with_empty_dock() { + let mut state = AppState::default(); + state.selected_room = Some(joined_room("!room:example.org", "octosbot")); + assert!( + state.saved_dock_state_home.open_rooms.is_empty(), + "precondition: this test simulates the mobile case where selected_room persists without desktop dock tabs", + ); + assert!( + state.saved_dock_state_home.dock_items.is_empty(), + "precondition: this test simulates the mobile case where selected_room persists without desktop dock tabs", + ); + + let serialized = + serde_json::to_string(&state).expect("AppState must serialize via serde_json"); + let deserialized: AppState = + serde_json::from_str(&serialized).expect("serialized AppState must deserialize back"); + + assert_eq!( + deserialized.selected_room, + Some(joined_room("!room:example.org", "octosbot")), + "selected_room must survive the round-trip even when dock state is empty", + ); + } } /// Actions sent to the top-level App in order to update / restore its [`AppState`]. diff --git a/src/sliding_sync.rs b/src/sliding_sync.rs index 56d662c7..4de0331c 100644 --- a/src/sliding_sync.rs +++ b/src/sliding_sync.rs @@ -1209,26 +1209,6 @@ mod matrix_request_tests { ); } - #[test] - fn test_should_restore_loaded_app_state_with_bot_settings_and_empty_dock() { - let mut app_state = crate::app::AppState::default(); - app_state.bot_settings.enabled = true; - app_state.bot_settings.botfather_user_id = "@octosbot:example.com".to_string(); - app_state.bot_settings.octos_service_url = "http://192.168.5.12:8010".to_string(); - - assert!( - should_restore_loaded_app_state(&app_state), - "non-default bot settings must restore even when dock state is empty", - ); - } - - #[test] - fn test_should_not_restore_loaded_default_app_state() { - assert!( - !should_restore_loaded_app_state(&crate::app::AppState::default()), - "fresh installs should keep in-memory defaults instead of dispatching a no-op restore", - ); - } } #[derive(Clone, Debug, PartialEq, Eq)] @@ -4973,24 +4953,11 @@ fn handle_ignore_user_list_subscriber(client: Client) { /// Asynchronously loads and restores the app state from persistent storage for the given user. /// -/// If the loaded dock state contains open rooms and dock items, this function emits an action -/// to instruct the UI to restore the app state for the main home view (all rooms). +/// Every successfully loaded `AppState` must be restored, including the all-default value. +/// That default payload is still semantically meaningful: it clears stale per-account state +/// during account switches and after corrupt-file fallback, while the restore handler keeps the +/// live `logged_in` flag and routes desktop dock reloads safely through `LoadDockFromAppState`. /// If loading fails, it shows a popup notification with the error message. -fn should_restore_loaded_app_state(app_state: &crate::app::AppState) -> bool { - let has_home_dock_state = - !app_state.saved_dock_state_home.open_rooms.is_empty() - || !app_state.saved_dock_state_home.dock_items.is_empty(); - let has_space_dock_state = app_state.saved_dock_state_per_space.values().any(|saved| { - !saved.open_rooms.is_empty() || !saved.dock_items.is_empty() - }); - - has_home_dock_state - || has_space_dock_state - || app_state.bot_settings != crate::app::BotSettingsState::default() - || app_state.app_language != crate::i18n::AppLanguage::default() - || app_state.translation != crate::room::translation::TranslationConfig::default() -} - fn handle_load_app_state(user_id: OwnedUserId) { Handle::current().spawn(async move { match take_skip_app_state_restore_once(&user_id).await { @@ -5006,16 +4973,8 @@ fn handle_load_app_state(user_id: OwnedUserId) { match load_app_state(&user_id).await { Ok(app_state) => { - // Issue #94: previously gated behind a non-empty dock-state check, which - // silently dropped bot_settings / app_language / translation on every mobile - // relaunch (mobile has no dock). Restore whenever any persisted state is - // meaningfully non-default, but keep the fresh-install path as a true no-op. - if should_restore_loaded_app_state(&app_state) { - log!("Loaded app state from persistent storage. Restoring now..."); - Cx::post_action(AppStateAction::RestoreAppStateFromPersistentState(Box::new(app_state))); - } else { - log!("Loaded default app state for {user_id}; nothing to restore."); - } + log!("Loaded app state from persistent storage. Restoring now..."); + Cx::post_action(AppStateAction::RestoreAppStateFromPersistentState(Box::new(app_state))); } Err(_e) => { log!("Failed to restore app state from persistent storage: {_e}");