MOB-113: extract Mob.Router and pin the hot-path property - #101
Merged
Conversation
MOB-112 gave Mob.Screen a third job. It was already the behaviour screens implement and the macro generating their boilerplate; it became the process owning navigation and every screen process too — nearly 1000 lines, with a moduledoc that had to describe all three, which is part of why that moduledoc has been wrong twice. Mob.Router now holds navigation, the screen processes, and the :mob_screen registered name. Mob.Screen keeps the behaviour and macro and delegates its public API, so Mob.Screen.dispatch/3 and friends are unchanged. A move, not a redesign: the process model landed in MOB-112 and is untouched. The property MOB-113 actually exists to guarantee — the router must not be in the per-message path — already held. MOB-111's listener delivers native events straight to the owning screen's pid, so a tap goes native -> listener -> screen -> sender with the router uninvolved. What is new is that it is asserted rather than reasoned about. router_hot_path_test.exs traces :receive on the router across a tap, a value-carrying event, and a burst of fifty messages, and asserts the trace is empty. Verified as a negative control: making the screen notify the router on each message fails exactly those three tests. That is worth a test rather than a comment. An earlier costing of this architecture assumed a router in the loop and concluded per-screen processes could not escape a hop per message; splitting the router from the sender is what dissolved that, and a property that load-bearing should fail loudly when someone breaks it. Also: decode_file_result/3 stopped being a @doc false public function and became private to Mob.Screen.Server, its only caller. And two guides were making claims that were aspirational before MOB-112 and are now nearly true — screen_lifecycle.md called each screen "a separate, supervised process", which is half right: separate yes, supervised no, because the router restarts screens itself since only it knows where a crashed one sat. Rationale in decisions/2026-08-29-router-off-the-hot-path.md. Tests: 5 new. Suite 1238 passed, format and credo --strict clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review found the extraction faithful — three hunks, every clause and attribute
intact, all seven entry points at every master arity — and one real problem,
which was mine: the test pinning this step's headline property was blind to
half the path.
Mob.Screen.Server.paint/3 short-circuits under :no_render, which is the only
mode host tests could reach. So tree expansion, ComponentRegistry.reconcile/2
and the hand-off to Mob.Sender never ran under trace, and a router hop added to
paint/3 passed the entire suite. That is exactly where a future hop would
appear — an "am I still active?" check in the render body is the obvious shape
of one.
The reviewer offered qualifying the claim or filing follow-up. Neither is good
enough for a commit whose deliverable is the test, so the gap is closed
instead: Mob.Screen.Server takes its NIF module as an option, defaulting to
:mob_nif. That is not test-only scaffolding — Mob.Renderer and Mob.Sender
already take it as a parameter and the screen was the outlier that hardcoded
it. With a stub NIF the test drives real renders off-device.
Both halves now bite, verified as negative controls: a hop in forward/2 fails
the three callback tests, a hop in paint/3 fails the two render tests. The
value-carrying test also gained the positive assertion it was missing — it
could previously have passed with its handler deleted.
Also from review, all documentation the rename left behind: screen/server.ex's
moduledoc still named Mob.Screen as the navigation owner (in a file this commit
edits, which would have been the third wrong moduledoc in this area), the same
in nav.ex and sender.ex, the comment twin in nav_test.exs that the commit fixed
in lib/ but not test/, and a user-facing ArgumentError still prefixed
"Mob.Screen:" for code that now lives in Mob.Router. Nav's moduledoc also still
described the pre-MOB-112 shape, claiming the current screen lives in
"Mob.Screen's {module, socket}".
Tests: 7 in the hot-path file, up from 5. Suite 1240 passed, 8/8 clean runs,
format and credo --strict clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>Uh oh!
There was an error while loading. Please reload this page.
GenericJam added a commit
that referenced
this pull request
Aug 29, 2026
Ports PR #100 onto master and fixes three deficiencies found reviewing it. `Mob.Socket.reset_to/4` takes `transition: :push | :pop | :reset | :none`. A reset always replaces the stack; the option only changes the animation, for cases like a custom tab bar where replacing the stack still represents directional movement. Ported, not merged: #100 patched `apply_nav_action/3` and `reset_resolved/4` in lib/mob/screen.ex, but MOB-113 (#101) has since extracted navigation into lib/mob/router.ex, where screen.ex no longer has an apply_nav_action at all. The change is re-applied against the router. Three fixes on top: 1. Mob.ScreenCase.navigated_to/1 matched only `{:reset, dest, _params}`, so once reset_to/4 started emitting a fourth element it fell through to the catch-all and returned the raw action tuple instead of the destination module. That broke `assert navigated_to(view) == SomeScreen` for EVERY reset — not just ones passing a transition, since the default also emits four elements — in the helper whose entire job is that assertion. No test covered reset there, which is why a green suite hid it. 2. The transition was unvalidated. set_transition/1 accepts any atom and the platform falls back to no animation for one it does not recognise, so a typo silently produced the wrong motion with nothing to point at. It is checked at the socket boundary now, matching how Mob.UI validates sheet detents. 3. The original tests asserted the nav action's shape but never that the chosen animation reaches the native boundary — a reset that recorded `:push` and still painted `:reset` would have passed them. Added router-level coverage in `:render` with an injected NIF, since do_paint short-circuits under `:no_render` and the transition is not observable there. The three-element action still works: it arrives from Mob.Test.reset_to/3 and from any socket built before a hot code push. Verified as negative controls — reverting the transition wiring fails the two directional tests, and reverting the ScreenCase clause fails both reset assertions. Suite 1254 passed, format and credo --strict clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
GenericJam added a commit
that referenced
this pull request
Aug 29, 2026
* feat(navigation): allow directional resets Ports PR #100 onto master and fixes three deficiencies found reviewing it. `Mob.Socket.reset_to/4` takes `transition: :push | :pop | :reset | :none`. A reset always replaces the stack; the option only changes the animation, for cases like a custom tab bar where replacing the stack still represents directional movement. Ported, not merged: #100 patched `apply_nav_action/3` and `reset_resolved/4` in lib/mob/screen.ex, but MOB-113 (#101) has since extracted navigation into lib/mob/router.ex, where screen.ex no longer has an apply_nav_action at all. The change is re-applied against the router. Three fixes on top: 1. Mob.ScreenCase.navigated_to/1 matched only `{:reset, dest, _params}`, so once reset_to/4 started emitting a fourth element it fell through to the catch-all and returned the raw action tuple instead of the destination module. That broke `assert navigated_to(view) == SomeScreen` for EVERY reset — not just ones passing a transition, since the default also emits four elements — in the helper whose entire job is that assertion. No test covered reset there, which is why a green suite hid it. 2. The transition was unvalidated. set_transition/1 accepts any atom and the platform falls back to no animation for one it does not recognise, so a typo silently produced the wrong motion with nothing to point at. It is checked at the socket boundary now, matching how Mob.UI validates sheet detents. 3. The original tests asserted the nav action's shape but never that the chosen animation reaches the native boundary — a reset that recorded `:push` and still painted `:reset` would have passed them. Added router-level coverage in `:render` with an injected NIF, since do_paint short-circuits under `:no_render` and the transition is not observable there. The three-element action still works: it arrives from Mob.Test.reset_to/3 and from any socket built before a hot code push. Verified as negative controls — reverting the transition wiring fails the two directional tests, and reverting the ScreenCase clause fails both reset assertions. Suite 1254 passed, format and credo --strict clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Address adversarial review; bump to 0.7.34 The finding that mattered: `:none` was in the allowed transition set, and it is the one value that suppresses the navigation-version bump — mob_nif's mob_bump_frame_generation, MobViewModel.navVersion, and the .id() on the root view. A reset stops every screen process and replaces the stack, so telling the platform no navigation happened leaves SwiftUI diffing the incoming tree into the outgoing screen's view identities: a text field at the same position inherits the old screen's text and focus, and scroll offsets survive a stack that no longer exists. Rejected now, with the reasoning recorded where the validator lives. Nothing can depend on it — the option is unreleased. The comment justifying the validator was also wrong about this. It argued from the platform's fallback for an unrecognised atom, which is strictly safer than the `:none` the validator was letting through. Also fixed: - An action shape the router does not recognise was an unmatched function clause in the owner, which owns navigation and links every live screen, so one bad action killed all of them. Reachable during a hot code push, where module loading is not atomic and a screen on new code can hand an action to a router on old code — the exact direction the compatibility clause cannot cover, because the guard would have to live in the old code. Now logged and ignored with a repaint. Pinned by a test; removing the clause fails it. - `Mob.ScreenCase.navigated_to/1`'s three-element clause was dead in-repo (Mob.Socket only emits four now), so both new tests exercised the same clause and deleting the legacy one left the suite green. The legacy test builds that shape by hand, which is the only way it now occurs — from a socket predating a hot code push. - `Mob.Test.reset_to/4` takes the option, so the behaviour is drivable on a device rather than only in-BEAM. - `@type transition` was declared and unreferenced while `@valid_transitions` duplicated it; the spec uses the type now. - Documented arities (`reset_to/2,3` in the guide and Mob.App), that the function can raise, and the test file leaking Mob.Sender/Mob.Listener under their global names. One test was a change-detector — it asserted the same four atoms the implementation uses as its allow-list, never consulting the renderer it named. Rewritten to pin the two things that matter: the transitions that work, and that :none does not. Suite 1256 passed, format and credo --strict clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 30, 2026
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 freeto 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.
Fifth step of epic MOB-108. Follows #95, #96, #97, #99.
Problem
MOB-112 gave
Mob.Screena third job. It was already the behaviour screens implement and the macro generating their boilerplate; it became the process owning navigation and every screen process too — nearly 1000 lines, with a moduledoc that had to describe all three. That's part of why that moduledoc has been wrong twice.Change
Mob.Routerholds navigation, the screen processes, and the:mob_screenregistered name.Mob.Screenkeeps the behaviour and macro and delegates its public API, soMob.Screen.dispatch/3and friends are unchanged.A move, not a redesign — MOB-112's process model is untouched. The review diffed it function by function: three hunks, every
handle_call/handle_info/handle_castclause in the same order, all module attributes intact, no helper orphaned or duplicated, and all seven entry points present at every arity they had before.The property this step exists to guarantee
The router must not be in the per-message path. It already wasn't — MOB-111's listener delivers native events straight to the owning screen's pid, so a tap goes native → listener → screen → sender with the router uninvolved.
What's new is that it's asserted rather than reasoned about.
router_hot_path_test.exstraces:receiveon the router and asserts the trace is empty.That's worth a test rather than a comment: an earlier costing of this architecture assumed a router in the loop and concluded per-screen processes couldn't escape a per-message hop. Splitting the router from the sender is what dissolved that, and a property carrying that much weight should fail loudly when someone breaks it.
What review caught
The first version of that test was blind to half the path.
paint/3short-circuits under:no_render, the only mode host tests could reach — so tree expansion,ComponentRegistry.reconcile/2and the hand-off toMob.Sendernever ran under trace, and a router hop added topaint/3passed the entire suite. That's precisely where a future hop would appear: an "am I still active?" check in the render body is the obvious shape of one.Closing it rather than qualifying it:
Mob.Screen.Servernow takes its NIF module as an option, defaulting to:mob_nif. Not test-only scaffolding —Mob.RendererandMob.Senderalready take it as a parameter and the screen was the outlier that hardcoded it. With a stub NIF the test drives real renders off-device.Both halves now bite, as negative controls: a hop in
forward/2fails the three callback tests, a hop inpaint/3fails the two render tests.Review also found every documentation reference the rename left behind — including
screen/server.ex's moduledoc still namingMob.Screenas the navigation owner, in a file this commit edits, which would have been the third wrong moduledoc in this area.What still goes through the router, deliberately
Navigation actions; the back gesture, alert actions and launch notifications that native addresses to
:mob_screen; device events forwarded to the active screen; andMob.Screen.dispatch/3. None is a per-message path for a running screen. The last two make the router a shared serialisation point — that's MOB-121.Verification
mix format,credo --strict,--warnings-as-errorsclean:mob_screenregistration is unmovedRationale in
decisions/2026-08-29-router-off-the-hot-path.md.