From 36ff3341270a863a41e991470bfc5cb563d37b17 Mon Sep 17 00:00:00 2001 From: GenericJam Date: Sat, 29 Aug 2026 12:08:52 -0600 Subject: [PATCH] fix(navigation): preserve initial transition --- lib/mob/router.ex | 47 +++++++++++-------- lib/mob/sender.ex | 64 ++++++++++++++++++++------ test/mob/screen_sender_wiring_test.exs | 8 ++-- test/mob/sender_test.exs | 64 ++++++++++++++++++++++++++ 4 files changed, 147 insertions(+), 36 deletions(-) diff --git a/lib/mob/router.ex b/lib/mob/router.ex index 12ef461c..975dd561 100644 --- a/lib/mob/router.ex +++ b/lib/mob/router.ex @@ -158,7 +158,7 @@ defmodule Mob.Router do case start_screen(screen_module, params, state) do {:ok, entry, state} -> - state = make_current(state, entry) + state = make_current(state, entry, :none) if render_mode == :render do # A notification that launched the app from a killed state. Sent to @@ -361,8 +361,8 @@ defmodule Mob.Router do # The single place `current` changes. The sender is told here and nowhere # else, so only the screen the user is looking at can commit a frame. - defp make_current(state, entry) do - Mob.Sender.set_active(entry.ref) + defp make_current(state, entry, transition) do + Mob.Sender.activate(entry.ref, transition) %{state | current: entry} end @@ -465,8 +465,8 @@ defmodule Mob.Router do Mob.Nav.history(state.nav) != [] -> state = drop_entry(state, dead_pid) [previous | rest] = Mob.Nav.history(state.nav) - state = make_current(%{state | nav: Mob.Nav.put_history(state.nav, rest)}, previous) - paint(previous, :pop, state) + state = make_current(%{state | nav: Mob.Nav.put_history(state.nav, rest)}, previous, :pop) + paint(previous, :none, state) state true -> @@ -485,8 +485,8 @@ defmodule Mob.Router do # switch/3 parks the dead entry under the outgoing stack; drop it straight # after, so nothing can restore a corpse by switching back. nav = Mob.Nav.drop_parked(nav, &(&1.pid == entry.pid)) - state = make_current(%{state | nav: nav}, live) - paint(live, :pop, state) + state = make_current(%{state | nav: nav}, live, :pop) + paint(live, :none, state) state else _ -> @@ -595,8 +595,11 @@ defmodule Mob.Router do # goes with it. The ones still in `rest` stay resident — that is what # makes pop restore prior state without re-mounting. state = stop_screen(state.current, state) - state = make_current(%{state | nav: Mob.Nav.put_history(state.nav, rest)}, previous) - do_paint(previous, :pop, state, mode) + + state = + make_current(%{state | nav: Mob.Nav.put_history(state.nav, rest)}, previous, :pop) + + do_paint(previous, :none, state, mode) state [] -> @@ -612,8 +615,8 @@ defmodule Mob.Router do ] state = Enum.reduce(discarded, state, &stop_screen/2) - state = make_current(%{state | nav: Mob.Nav.put_history(state.nav, [])}, root) - do_paint(root, :pop, state, mode) + state = make_current(%{state | nav: Mob.Nav.put_history(state.nav, [])}, root, :pop) + do_paint(root, :none, state, mode) state [] -> @@ -643,7 +646,7 @@ defmodule Mob.Router do defp apply_nav_action({:switch_tab, tab}, state, mode) do case Mob.Nav.switch(state.nav, tab, state.current) do {:switched, nav, entry} -> - state = make_current(%{state | nav: nav}, entry) + state = make_current(%{state | nav: nav}, entry, :none) do_paint(entry, :none, state, mode) state @@ -652,7 +655,7 @@ defmodule Mob.Router do # leaves navigation pointing at a stack whose screen never started. case start_screen(root_module, %{}, state) do {:ok, entry, state} -> - state = make_current(%{state | nav: nav}, entry) + state = make_current(%{state | nav: nav}, entry, :none) do_paint(entry, :none, state, mode) state @@ -684,8 +687,8 @@ defmodule Mob.Router do case start_screen(new_module, mount_params, state) do {:ok, entry, state} -> nav = Mob.Nav.put_history(state.nav, [state.current | Mob.Nav.history(state.nav)]) - state = make_current(%{state | nav: nav}, entry) - do_paint(entry, :push, state, mode) + state = make_current(%{state | nav: nav}, entry, :push) + do_paint(entry, :none, state, mode) state {:error, _reason} -> @@ -698,8 +701,11 @@ defmodule Mob.Router do {:ok, entry, state} -> discarded = [state.current | Mob.Nav.history(state.nav)] state = Enum.reduce(discarded, state, &stop_screen/2) - state = make_current(%{state | nav: Mob.Nav.put_history(state.nav, [])}, entry) - do_paint(entry, transition, state, mode) + + state = + make_current(%{state | nav: Mob.Nav.put_history(state.nav, [])}, entry, transition) + + do_paint(entry, :none, state, mode) state {:error, _reason} -> @@ -714,8 +720,11 @@ defmodule Mob.Router do {:found, previous, rest} -> discarded = [state.current | Enum.take_while(history, &(&1.pid != previous.pid))] state = Enum.reduce(discarded, state, &stop_screen/2) - state = make_current(%{state | nav: Mob.Nav.put_history(state.nav, rest)}, previous) - do_paint(previous, :pop, state, mode) + + state = + make_current(%{state | nav: Mob.Nav.put_history(state.nav, rest)}, previous, :pop) + + do_paint(previous, :none, state, mode) state :not_found -> diff --git a/lib/mob/sender.ex b/lib/mob/sender.ex index cce99070..ab298da4 100644 --- a/lib/mob/sender.ex +++ b/lib/mob/sender.ex @@ -50,6 +50,11 @@ defmodule Mob.Sender do `sync/1` that merely replied would return before the frame was committed. Mailbox order is the wrong tool here, and it looks like the right one. + `Mob.Router` uses `activate/2` before asking a screen to paint. Activation is + synchronous and carries the navigation transition as a one-shot reservation, + so an ordinary repaint from the newly active screen cannot race ahead and + erase the animation. Whichever tree arrives first consumes the reservation. + `Mob.Router` uses `sync/1` on its `handle_call` paths to keep the guarantee `Mob.Test` documents for the synchronous navigation helpers. Note the ordering guarantee only covers renders cast by the *calling* process; the BEAM promises @@ -68,7 +73,7 @@ defmodule Mob.Sender do """ @type screen_ref :: reference() | atom() - defstruct active: nil, pending: %{} + defstruct active: nil, pending: %{}, reserved_transition: nil @doc "Start the sender. Named, so there is exactly one." @spec start_link(keyword()) :: GenServer.on_start() @@ -114,6 +119,21 @@ defmodule Mob.Sender do @spec set_active(screen_ref()) :: :ok def set_active(ref), do: GenServer.cast(__MODULE__, {:set_active, ref}) + @doc """ + Activate a screen and reserve its navigation transition for the next frame. + + Unlike `set_active/1`, this call is synchronous. The router uses it at the + navigation boundary so paints sent by different screen processes cannot be + observed before the transition intent. The first tree for `ref` consumes the + reservation; later ordinary repaints remain `:none`. + + A `:none` transition only activates the screen and creates no reservation. + """ + @spec activate(screen_ref(), atom()) :: :ok + def activate(ref, transition) do + if running?(), do: GenServer.call(__MODULE__, {:activate, ref, transition}), else: :ok + end + @doc """ Queue `tree` for commit on behalf of screen `ref`. @@ -146,7 +166,23 @@ defmodule Mob.Sender do end @impl GenServer - def handle_cast({:set_active, ref}, state), do: {:noreply, %{state | active: ref}} + def handle_call({:activate, ref, transition}, _from, state) do + reserved_transition = if transition == :none, do: nil, else: {ref, transition} + {:reply, :ok, %{state | active: ref, reserved_transition: reserved_transition}} + end + + def handle_call(:sync, _from, state) do + # Flush here rather than just replying. The `:flush` this render self-sent + # lands at the BACK of the mailbox, which is behind a `sync` the caller has + # already queued — replying without flushing would return before the frame + # was committed, which is the one thing this function promises not to do. + {:reply, :ok, flush(state)} + end + + @impl GenServer + def handle_cast({:set_active, ref}, state) do + {:noreply, %{state | active: ref, reserved_transition: nil}} + end def handle_cast({:render, ref, tree, platform, nif, transition}, state) do # Overwrite rather than append: a newer tree for the same screen supersedes @@ -154,12 +190,23 @@ defmodule Mob.Sender do # is the exception — it describes the navigation animation for this frame, # not the frame's content, so a push superseded by an ordinary re-render # still has to animate as a push or the transition is silently swallowed. - transition = carry_transition(state.pending, ref, transition) + {transition, reserved_transition} = + take_transition(state.pending, state.reserved_transition, ref, transition) + pending = Map.put(state.pending, ref, {tree, platform, nif, transition}) send(self(), :flush) - {:noreply, %{state | pending: pending}} + {:noreply, %{state | pending: pending, reserved_transition: reserved_transition}} end + defp take_transition(pending, {ref, reserved}, ref, :none), + do: {carry_transition(pending, ref, reserved), nil} + + defp take_transition(pending, {ref, _reserved}, ref, transition), + do: {carry_transition(pending, ref, transition), nil} + + defp take_transition(pending, reserved, ref, transition), + do: {carry_transition(pending, ref, transition), reserved} + defp carry_transition(pending, ref, :none) do case Map.fetch(pending, ref) do {:ok, {_tree, _platform, _nif, superseded}} -> superseded @@ -174,15 +221,6 @@ defmodule Mob.Sender do def handle_info(_message, state), do: {:noreply, state} - @impl GenServer - def handle_call(:sync, _from, state) do - # Flush here rather than just replying. The `:flush` this render self-sent - # lands at the BACK of the mailbox, which is behind a `sync` the caller has - # already queued — replying without flushing would return before the frame - # was committed, which is the one thing this function promises not to do. - {:reply, :ok, flush(state)} - end - defp flush(state) do case Map.fetch(state.pending, state.active) do {:ok, payload} -> commit(payload) diff --git a/test/mob/screen_sender_wiring_test.exs b/test/mob/screen_sender_wiring_test.exs index 771e5f69..a0741efe 100644 --- a/test/mob/screen_sender_wiring_test.exs +++ b/test/mob/screen_sender_wiring_test.exs @@ -2,9 +2,9 @@ defmodule Mob.ScreenSenderWiringTest do @moduledoc """ The seam between `Mob.Screen` and `Mob.Sender`. - Screens run `:no_render` here, so no tree is ever committed — but - `Mob.Sender.set_active/1` is a cast, so with a real sender running these - assertions pin *who* declares the active screen and *when*. + Screens run `:no_render` here, so no tree is ever committed — but with a real + sender running these assertions pin *who* declares the active screen and + *when*. Since MOB-112 the sender's key is per **screen**, not per stack. Every screen is a live process that repaints on any message it receives, including the ones @@ -131,7 +131,7 @@ defmodule Mob.ScreenSenderWiringTest do Sender.sync() settings_ref = current_ref(screen) - # A screen re-rendering must not promote itself — this fails if set_active/1 + # A screen re-rendering must not promote itself — this fails if activation # moves back into the render path. Mob.Screen.dispatch(screen, "bump", %{}) Sender.sync() diff --git a/test/mob/sender_test.exs b/test/mob/sender_test.exs index 0dbce39a..4ebdebc5 100644 --- a/test/mob/sender_test.exs +++ b/test/mob/sender_test.exs @@ -144,6 +144,70 @@ defmodule Mob.SenderTest do end describe "coalescing preserves the transition" do + test "an immediate first paint cannot overtake its navigation transition" do + start_sender(:home) + + assert :ok = Sender.activate(:details, :push) + Sender.render(:details, tree("mounted"), :android, RecordingNif, :none) + Sender.sync() + + Sender.render(:details, tree("loaded"), :android, RecordingNif, :none) + Sender.sync() + + transitions = for {:set_transition, transition} <- RecordingNif.calls(), do: transition + + assert transitions == [:push, :none] + + assert [mounted, loaded] = committed_texts() + assert mounted =~ "mounted" + assert loaded =~ "loaded" + end + + test "an ordinary first paint consumes the activated navigation transition" do + state = %Sender{active: :home} + + {:reply, :ok, state} = Sender.handle_call({:activate, :details, :push}, self(), state) + + {:noreply, state} = + Sender.handle_cast({:render, :details, tree("first"), :ios, RecordingNif, :none}, state) + + assert state.reserved_transition == nil + {:noreply, _state} = Sender.handle_info(:flush, state) + + assert {:set_transition, :push} in RecordingNif.calls() + end + + test "the activated transition survives a second ordinary paint before flush" do + state = %Sender{active: :home} + + {:reply, :ok, state} = Sender.handle_call({:activate, :details, :push}, self(), state) + + {:noreply, state} = + Sender.handle_cast({:render, :details, tree("first"), :ios, RecordingNif, :none}, state) + + {:noreply, state} = + Sender.handle_cast({:render, :details, tree("latest"), :ios, RecordingNif, :none}, state) + + {:noreply, _state} = Sender.handle_info(:flush, state) + + assert {:set_transition, :push} in RecordingNif.calls() + assert [json] = committed_texts() + assert json =~ "latest" + end + + test "activation without a transition leaves the first paint ordinary" do + state = %Sender{active: :home} + + {:reply, :ok, state} = Sender.handle_call({:activate, :settings, :none}, self(), state) + + {:noreply, state} = + Sender.handle_cast({:render, :settings, tree("first"), :ios, RecordingNif, :none}, state) + + {:noreply, _state} = Sender.handle_info(:flush, state) + + assert {:set_transition, :none} in RecordingNif.calls() + end + test "a push superseded by an ordinary re-render still animates as a push" do state = %Sender{active: :home}