From aa1d109a3086375de183cdeed8d090a1e833d782 Mon Sep 17 00:00:00 2001 From: GenericJam Date: Sat, 29 Aug 2026 05:02:44 -0600 Subject: [PATCH] MOB-114: pin the two behaviours that arrived for free MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Scoped as three pieces of migration work. Two were already done: each live screen has scheduled its own state sync and dumped in its own terminate/2 since MOB-112, and hot reload has been a broadcast over every live screen since the same change. They fell out of making screens processes rather than needing separate work. Neither had a test. That is the real gap — behaviour that arrived as a side effect of another change, asserted nowhere. Reviews of MOB-112 flagged hot_reload as having zero coverage, and nothing had ever round-tripped state across a multi-stack app, which is this issue's acceptance criterion. The third piece was attempted and REVERTED, which is the more useful result. Mob.Test's tap/select/send_message were changed to resolve the screen pid and send directly, on the reasoning that a native tap reaches a screen via Mob.Listener without touching the router. Review showed that was wrong three ways: - It made the thing it was avoiding worse. get_screen_pid/1 is a GenServer.call INTO the router — the serialisation point the change cited as motivation — replacing one async send with a synchronous round-trip plus a second send. Measured at 5002ms for one send_message while the router was blocked, against a documented fire-and-forget contract. - Resolve-then-send is not atomic. The router reads state.current.pid and delivers in one step; resolving separately opens a full RPC round trip in which the screen can restart and the event be delivered to a corpse. - The premise was factually wrong. Alert actions go to :mob_screen on both platforms and webview messages on iOS unconditionally, and both are documented send_message/2 payloads — so router-handled shapes stopped working entirely. send_message({:mob, :back}) no longer popped. Addressing :mob_screen is correct, not just the status quo: the router resolves and delivers atomically in one non-blocking RPC, and it is where native itself sends the messages this function simulates. lib/ is unchanged by this step. Three barriers looked like barriers and were not, each producing a passing test that proved nothing: - Mob.Sender.sync/1 does not order a hot reload — a background screen's tree is dropped rather than committed. Nor does :sys.get_state(router), since hot_reload/1 is a cast per screen. The screens are drained, and because render/1 is user code the assertion is a bounded wait rather than a snapshot comparison: with a 30ms sleep in render/1 the snapshot version failed every run and the bounded one passes. - Screens dump in their own terminate/2 after the router exits, so GenServer.stop(router) is not enough; the state tests monitor the screens and wait for their exits, or the dump races the Repo teardown and logs a DB error out of a passing test (reproduced 10/15 without the fix). - all_entries/1 has three branches — current, the ACTIVE stack's history, and parked — and the first scenario ended on a tab switch, leaving the active history empty. Deleting that branch outright passed the whole suite. The scenario now switches back, so all three are populated, and dropping either the history or the parked branch fails the test. Ordering: audited. On any message path the only multi-screen iteration is the hot-reload broadcast; the stop_screen reductions in pop_to_root, pop_to and reset are order-independent. Rationale in decisions/2026-08-29-migration-off-the-single-screen-process.md. Tests: 3 new. Suite 1243 passed, 20/20 clean runs, format and credo --strict clean. Co-Authored-By: Claude Opus 5 (1M context) --- ...migration-off-the-single-screen-process.md | 102 ++++++ test/mob/screen/migration_test.exs | 313 ++++++++++++++++++ 2 files changed, 415 insertions(+) create mode 100644 decisions/2026-08-29-migration-off-the-single-screen-process.md create mode 100644 test/mob/screen/migration_test.exs diff --git a/decisions/2026-08-29-migration-off-the-single-screen-process.md b/decisions/2026-08-29-migration-off-the-single-screen-process.md new file mode 100644 index 00000000..a026087b --- /dev/null +++ b/decisions/2026-08-29-migration-off-the-single-screen-process.md @@ -0,0 +1,102 @@ +# Migration: the surface that assumed one screen process + +- Date: 2026-08-29 +- Status: accepted +- Implements: MOB-114, final step of MOB-108 +- Builds on: `2026-08-28-screen-processes-and-supervision.md`, + `2026-08-29-router-off-the-hot-path.md` + +## Context + +MOB-114 was scoped as three pieces of migration work: `Mob.Test`'s direct +`:mob_screen` calls, `dump_state`/`load_state` spanning processes, and +`__mob_hot_reload__` becoming a broadcast. + +Two of the three were already done when this step started. They fell out of +MOB-112 rather than needing separate work: each live screen schedules its own +state sync and dumps in its own `terminate/2`, and hot reload has been a +broadcast over `all_entries/1` since screens became processes. + +Neither had a test. That is the real gap — behaviour that arrived as a side +effect of another change, asserted nowhere. + +## Decision + +### `Mob.Test` keeps addressing `:mob_screen` + +The third piece was attempted and **reverted**, which is the more useful thing +to record. + +`tap/2`, `select/3` and `send_message/2` were changed to resolve the screen pid +and send to it directly, on the reasoning that a native tap reaches a screen via +`Mob.Listener` without touching the router, so the harness should too. Review +showed that reasoning was wrong on three counts: + +* **It made the thing it was avoiding worse.** `Mob.Screen.get_screen_pid/1` is + a `GenServer.call` into the router — the same serialisation point the change + cited as its motivation. It replaced one asynchronous send through the + router's mailbox with a synchronous round-trip into it, plus a second send. + Measured at **5002 ms** for one `send_message` while the router was blocked in + a call to a screen, against a documented fire-and-forget contract. +* **Resolve-then-send is not atomic.** The router's forward reads + `state.current.pid` and delivers in one step. Resolving separately opens a + window — a full RPC round trip, tens of milliseconds over a tunnel — in which + the screen can be restarted and the event delivered to a corpse. Proved: kill + the screen between resolve and send and the message is lost, where addressing + `:mob_screen` delivers it to the live replacement. +* **The premise was factually wrong.** Native delivers tap-handle events to the + screen, but alert actions go to `:mob_screen` on both platforms + (`ios/mob_nif.m`, `android/jni/mob_nif.zig`), and webview messages do on iOS + unconditionally and on Android whenever no explicit pid was registered. Those + are documented `send_message/2` payloads. Router-handled shapes stopped working + entirely: `send_message(node, {:mob, :back})` no longer popped, because the + screen's default `handle_info` swallowed it. + +Addressing `:mob_screen` is correct, and not merely the status quo: the router +resolves and delivers atomically, in one non-blocking RPC, and it is where +native itself sends the messages this function is documented to simulate. + +`back/1` and `navigate/2` were always right for the same reason. + +### The two behaviours that arrived for free are now pinned + +`test/mob/screen/migration_test.exs` asserts that hot reload repaints a screen +in a parked stack's history *and* that stack's current screen, not just the one +on screen; and that a screen in history persists and restores its own assigns. + +Both verified as negative controls: reverting hot reload to a single cast, or +removing the dump from `Mob.Screen.Server.terminate/2`, fails exactly the tests +written for them. Before MOB-112 the second was not merely untested but +impossible — only the active screen held a socket. + +### Two barriers that looked like barriers and were not + +Worth recording, because both produced a passing test that proved nothing. + +`Mob.Sender.sync/1` is not a barrier for hot reload. A background screen's tree +is *dropped* rather than committed, so the sender catching up says nothing about +whether that screen processed its cast. Neither is `:sys.get_state(router)`: +`hot_reload/1` is a cast per screen, so draining the router proves only that the +casts were sent. The screens themselves have to be drained — and because +`render/1` is user code that may take arbitrarily long, the assertion is a +bounded wait rather than a snapshot comparison. With a 30 ms sleep in `render/1` +the snapshot version failed every run; the bounded version passes. + +Screens dump in their own `terminate/2`, which runs *after* the router exits, so +`GenServer.stop(router)` returning does not mean the writes have landed. The +state tests monitor the screens and wait for their exits — otherwise the dump +races `start_supervised!(Repo)`'s teardown and logs a DB error out of a passing +test. + +## Consequences + +- **Ordering weakened from total to per-screen, and nothing depended on it.** + Audited: on any *message* path the only multi-screen iteration in `lib/` is the + hot-reload broadcast, where each screen repaints independently and only the + active one's tree is committed. (`handle_call(:get_nav_history, …)` also walks + every history entry, but it is a debugging API rather than an event path, and + it only reads.) The `Enum.reduce(discarded, …, &stop_screen/2)` + calls in `pop_to_root`, `pop_to` and `reset` are sequential but + order-independent — each entry is stopped in isolation. +- `Mob.Test` is unchanged by this step. The epic listed its seven `:mob_screen` + call sites as migration work; the conclusion is that they were already right. diff --git a/test/mob/screen/migration_test.exs b/test/mob/screen/migration_test.exs new file mode 100644 index 00000000..6a03087b --- /dev/null +++ b/test/mob/screen/migration_test.exs @@ -0,0 +1,313 @@ +defmodule Mob.Screen.MigrationTest do + @moduledoc """ + The surface that was coupled to there being one screen process. + + Hot reload had to become a broadcast, and `dump_state`/`load_state` had to + span processes — a screen parked under an inactive tab holds its own socket + now, so only that process can persist it. Both fell out of MOB-112, and + neither had a test. + """ + use ExUnit.Case, async: false + + # ── Hot reload ──────────────────────────────────────────────────────────── + + defmodule Renders do + def start, do: Agent.start(fn -> %{} end, name: __MODULE__) + def count(module), do: Agent.get(__MODULE__, &Map.get(&1, module, 0)) + def note(module), do: Agent.update(__MODULE__, &Map.update(&1, module, 1, fn n -> n + 1 end)) + end + + defmodule StubNif do + def platform, do: :android + def safe_area, do: {0.0, 0.0, 0.0, 0.0} + def take_launch_notification, do: :none + def clear_taps, do: :ok + def set_transition(_), do: :ok + def register_tap(_), do: 0 + def set_root(_json), do: :ok + end + + defmodule HomeScreen do + use Mob.Screen + + @detail Mob.Screen.MigrationTest.DetailScreen + + def mount(_p, _s, socket), do: {:ok, socket} + + def render(_assigns) do + Mob.Screen.MigrationTest.Renders.note(__MODULE__) + %{type: :text, props: %{text: "home"}, children: []} + end + + def handle_event("push", _, socket), do: {:noreply, Mob.Socket.push_screen(socket, @detail)} + + def handle_event("to_settings", _, socket), + do: {:noreply, Mob.Socket.switch_tab(socket, :settings)} + + def handle_event("to_home", _, socket), + do: {:noreply, Mob.Socket.switch_tab(socket, :home)} + end + + defmodule DetailScreen do + use Mob.Screen + def mount(_p, _s, socket), do: {:ok, socket} + + def render(_assigns) do + Mob.Screen.MigrationTest.Renders.note(__MODULE__) + %{type: :text, props: %{text: "detail"}, children: []} + end + + def handle_event("to_settings", _, socket), + do: {:noreply, Mob.Socket.switch_tab(socket, :settings)} + end + + defmodule SettingsScreen do + use Mob.Screen + def mount(_p, _s, socket), do: {:ok, socket} + + def render(_assigns) do + Mob.Screen.MigrationTest.Renders.note(__MODULE__) + %{type: :text, props: %{text: "settings"}, children: []} + end + + def handle_event("to_home", _, socket), + do: {:noreply, Mob.Socket.switch_tab(socket, :home)} + end + + defmodule TabApp do + @behaviour Mob.App + import Mob.App + @home Mob.Screen.MigrationTest.HomeScreen + @settings Mob.Screen.MigrationTest.SettingsScreen + def navigation(_), do: tab_bar([stack(:home, root: @home), stack(:settings, root: @settings)]) + end + + defp stop_safely(pid) do + GenServer.stop(pid) + catch + :exit, _ -> :ok + end + + # Screens dump in their own terminate/2, which runs after the router exits — + # so the router being down does not mean the writes have landed. + defp stop_and_await_screens(router) do + refs = for pid <- live_screen_pids(router), do: {pid, Process.monitor(pid)} + stop_safely(router) + + for {pid, ref} <- refs do + receive do + {:DOWN, ^ref, :process, ^pid, _} -> :ok + after + 2_000 -> flunk("screen #{inspect(pid)} did not terminate") + end + end + end + + # Every live screen: current, the active stack's history, and everything + # parked under an inactive stack. + defp live_screen_pids(router) do + state = :sys.get_state(router) + + parked = + state.nav + |> Map.get(:parked, %{}) + |> Enum.flat_map(fn {_name, %{current: c, history: h}} -> [c | h] end) + + ([state.current] ++ Mob.Nav.history(state.nav) ++ parked) + |> Enum.map(& &1.pid) + |> Enum.uniq() + end + + # Hot reload is a broadcast of casts, and a screen's render/1 is user code + # that may take as long as it likes. Draining each screen covers the common + # case; this covers the rest without pinning the test to how fast render/1 is. + defp wait_until(fun, timeout \\ 2_000) do + deadline = System.monotonic_time(:millisecond) + timeout + do_wait_until(fun, deadline) + end + + defp do_wait_until(fun, deadline) do + cond do + fun.() -> :ok + System.monotonic_time(:millisecond) > deadline -> false + true -> Process.sleep(5) && do_wait_until(fun, deadline) + end + end + + defp reset_services do + for name <- [Mob.Sender, Mob.Listener, Mob.ComponentRegistry, Mob.Nav.Registry], + pid = Process.whereis(name), + do: stop_safely(pid) + end + + describe "hot reload reaches every live screen" do + setup do + reset_services() + + if pid = Process.whereis(Renders), do: Agent.stop(pid) + {:ok, renders} = Renders.start() + # Globally named and unlinked, so it outlives the suite unless stopped. + on_exit(fn -> if Process.alive?(renders), do: Agent.stop(renders) end) + + {:ok, _} = Mob.ComponentRegistry.start_link() + {:ok, _} = Mob.Nav.Registry.start_link(TabApp) + {:ok, router} = Mob.Router.start_root(HomeScreen, %{}, nif: StubNif) + + on_exit(fn -> + stop_safely(router) + reset_services() + end) + + %{router: router} + end + + test "a screen in history and one parked under another tab both repaint", %{router: router} do + # all_entries/1 has three branches — current, the ACTIVE stack's history, + # and everything parked — so the scenario has to populate all three or a + # deleted branch goes unnoticed. Push, switch away (parking the home stack + # and mounting settings), then switch back: home is active with Detail + # current and Home in its history, and the settings stack is parked. + modules = [HomeScreen, DetailScreen, SettingsScreen] + + Mob.Screen.dispatch(router, "push", %{}) + Mob.Screen.dispatch(router, "to_settings", %{}) + Mob.Screen.dispatch(router, "to_home", %{}) + + # Every first paint is an async cast, so settle those before taking the + # baseline or it races the mount renders. + assert wait_until(fn -> Enum.all?(modules, &(Renders.count(&1) > 0)) end) == :ok + before = for m <- modules, into: %{}, do: {m, Renders.count(m)} + + screens = live_screen_pids(router) + assert length(screens) == 3 + + state = :sys.get_state(router) + assert state.current.module == DetailScreen + + assert [%{module: HomeScreen}] = Mob.Nav.history(state.nav), + "active history must be populated" + + assert Map.has_key?(state.nav.parked, :settings), "a parked stack must be populated" + + GenServer.cast(router, :__mob_hot_reload__) + + # Neither :sys.get_state(router) nor Mob.Sender.sync/1 is a barrier here. + # hot_reload/1 is a cast per screen, so draining the router proves only + # that the casts were sent; and a background screen's tree is dropped + # rather than committed, so the sender catching up says nothing either. + # Drain the screens, then wait — render/1 is user code and may be slow. + :sys.get_state(router) + Enum.each(screens, &:sys.get_state/1) + + assert wait_until(fn -> Enum.all?(modules, &(Renders.count(&1) > before[&1])) end) == :ok, + "not every live screen repainted on hot reload: " <> + inspect(for m <- modules, into: %{}, do: {m, {before[m], Renders.count(m)}}) + end + end + + # ── State restore across processes ──────────────────────────────────────── + + defmodule Repo do + use Ecto.Repo, otp_app: :mob_migration_test, adapter: Ecto.Adapters.SQLite3 + end + + @create_table """ + CREATE TABLE IF NOT EXISTS mob_screen_states ( + key TEXT PRIMARY KEY NOT NULL, + vsn INTEGER NOT NULL DEFAULT 0, + data BLOB NOT NULL, + updated_at INTEGER NOT NULL + ) + """ + + defmodule PersistHome do + use Mob.Screen, vsn: 1 + @detail Mob.Screen.MigrationTest.PersistDetail + + def mount(_p, _s, socket), do: {:ok, Mob.Socket.assign(socket, :note, "home-default")} + def render(_a), do: %{type: :text, props: %{text: "h"}, children: []} + + def handle_event("mark", _, socket), + do: {:noreply, Mob.Socket.assign(socket, :note, "home-kept")} + + def handle_event("push", _, socket), do: {:noreply, Mob.Socket.push_screen(socket, @detail)} + end + + defmodule PersistDetail do + use Mob.Screen, vsn: 1 + def mount(_p, _s, socket), do: {:ok, Mob.Socket.assign(socket, :note, "detail-default")} + def render(_a), do: %{type: :text, props: %{text: "d"}, children: []} + + def handle_event("mark", _, socket), + do: {:noreply, Mob.Socket.assign(socket, :note, "detail-kept")} + end + + defmodule PersistApp do + @behaviour Mob.App + import Mob.App + @home Mob.Screen.MigrationTest.PersistHome + def navigation(_), do: stack(:home, root: @home) + end + + describe "state restore spans every live screen's process" do + setup do + reset_services() + db = System.tmp_dir!() <> "/mob_migration_#{System.unique_integer([:positive])}.db" + Application.put_env(:mob_migration_test, Repo, database: db, pool_size: 1) + Application.put_env(:mob, :repo, Repo) + + start_supervised!(Repo) + Repo.query!(@create_table, []) + + {:ok, _} = Mob.Nav.Registry.start_link(PersistApp) + + on_exit(fn -> + Application.delete_env(:mob, :repo) + Application.delete_env(:mob_migration_test, Repo) + reset_services() + File.rm(db) + end) + + :ok + end + + test "a screen in history persists too, not just the one on screen" do + # Before MOB-112 only the active screen held a socket, so only it could + # ever be dumped — a screen you had navigated away from lost its state. + {:ok, router} = Mob.Screen.start_link(PersistHome, %{}) + Mob.Screen.dispatch(router, "mark", %{}) + Mob.Screen.dispatch(router, "push", %{}) + Mob.Screen.dispatch(router, "mark", %{}) + + stop_and_await_screens(router) + + %{rows: rows} = Repo.query!("SELECT key FROM mob_screen_states", []) + keys = List.flatten(rows) + + assert to_string(PersistHome) in keys, "the screen in history was never dumped" + assert to_string(PersistDetail) in keys + end + + test "both screens restore their own assigns on relaunch" do + {:ok, router} = Mob.Screen.start_link(PersistHome, %{}) + Mob.Screen.dispatch(router, "mark", %{}) + Mob.Screen.dispatch(router, "push", %{}) + Mob.Screen.dispatch(router, "mark", %{}) + stop_and_await_screens(router) + + # Relaunch: mount runs again, then load_state/2 puts the dumped assigns back. + {:ok, router} = Mob.Screen.start_link(PersistHome, %{}) + + assert Mob.Screen.get_socket(router).assigns.note == "home-kept" + + Mob.Screen.dispatch(router, "push", %{}) + assert Mob.Screen.get_socket(router).assigns.note == "detail-kept" + + # Await the screens, not just the router: they dump in their own + # terminate/2, which runs after the router exits. start_supervised!(Repo) + # is torn down before on_exit, so an unawaited dump writes into a closed + # connection and logs an error out of a passing test. + stop_and_await_screens(router) + end + end +end