From e998740c0e530ec1cf3c7607a6b6f181100ddb7c Mon Sep 17 00:00:00 2001 From: GenericJam Date: Sat, 5 Sep 2026 14:27:06 -0600 Subject: [PATCH 1/4] Let Mob.RenderStats see the native half of an Android frame (MOB-146) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit native_summary/1 returned {:error, :unsupported} on Android because there was no native implementation, which looks identical to "you forgot to enable it". Adds native_stats/0 and native_stats_enable/1 there. The Erlang stubs and all of Mob.RenderStats were already platform-agnostic, so only the native half was missing. The buffer lives in the app's generated MobBridge.kt rather than here: the measurement can only be taken on the main thread, so keeping it beside the writer avoids a JNI hop per sample. These NIFs fetch and toggle it the way nif_element_frames fetches frames, and both Kotlin methods are cacheOptional so an app generated before they existed still loads. First Android navigation baseline, physical moto g power, 1600-node screen: none 221ms p50, push 818ms, pop 788ms. Navigation costs 3.7x a re-render of the same tree — the dispose-and-recreate gap this ticket exists to close, now a number rather than an inference. Corroborated against the platform's own accounting before it was believed: "Davey! duration=1414ms" and "Choreographer: Skipped 67 frames" in the same window. Android's apply_us is NOT iOS's apply_us, and the decision record says so plainly because a differential test would otherwise compare them and conclude something false. iOS starts its clock on the main thread after the dispatch hop; Android starts on the NIF thread, so its interval includes a thread handoff and up to one vsync of queue latency. It is a before-and-after tool for one platform, not a cross-platform comparison. Review fixes: - native_call/3 now converts a RETURNED {:error, :not_loaded} to :unsupported. Android signals a missing bridge method by returning rather than raising, so it took the one path that was not converted, leaking a fourth shape past a @spec promising two — and every app generated before this release hits it. - Exact atom compare in native_stats_enable rather than a four-byte prefix, which accepted :truthy and :true_x as "on" — the typo-enables-it case the comment beside it claimed to refuse. Anything that is neither true nor false is now badarg, matching iOS. - native_stats added to iOS's caps[] and to Mob.Test.probe_keys/0. An existing test enforces three-way agreement between the two platforms and Elixir, and caught this: a key advertised by one native half and not the other. - Moved a comment that had landed above sample_region's value instead of render_stats', inside the exact positional array it was describing. The capabilities test now checks the arrays pairwise rather than by length. Counting cannot catch the failure it exists to catch: inserting a key mid-array while appending its value at the end keeps the counts equal and produces exactly the misordering, where each capability reports a different NIF's availability. Mutation-checked by swapping two values. Also pins the Android JSON shape against Mob.RenderStats: a wrapped ring (recorded 250, retained 240, dropped 10), an escaped quote in a transition, and an unescaped one — the last asserting that it costs the whole window rather than one sample, so the escaping in MobBridge.kt reads as load-bearing. Pairs with mob_new. Suite 1545 passed, credo clean. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 25 +++++ android/jni/mob_nif.zig | 82 +++++++++++++++++ ...9-05-measure-the-native-half-on-android.md | 92 +++++++++++++++++++ ios/mob_nif.m | 3 + lib/mob/render_stats.ex | 22 ++++- lib/mob/test.ex | 3 +- test/mob/android_native_stats_test.exs | 80 ++++++++++++++++ test/mob/native_frame_stats_test.exs | 76 ++++++++++++++- 8 files changed, 375 insertions(+), 8 deletions(-) create mode 100644 decisions/2026-09-05-measure-the-native-half-on-android.md create mode 100644 test/mob/android_native_stats_test.exs diff --git a/CHANGELOG.md b/CHANGELOG.md index ce1cea7..c397cdb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,31 @@ Full module documentation: [hexdocs.pm/mob](https://hexdocs.pm/mob). ## [Unreleased] ### Added +- **Native frame timing on Android.** `Mob.RenderStats.native_enable/1`, + `native_frames/1` and `native_summary/1` returned `{:error, :unsupported}` on + Android, which looks identical to "you forgot to enable it". They now work, + emitting the same JSON shape iOS does so nothing needs per-platform parsing. + + First baseline, physical moto g power, 1600-node screen: a one-field + re-render costs 266ms p50, a `push` 841ms and a `pop` 916ms — navigation is + **3.2x** a re-render of the same tree. See + `decisions/2026-09-05-measure-the-native-half-on-android.md`, which also + records why the obvious closing brackets (`MessageQueue.IdleHandler`, a + plain `post`) measure the wrong thing without failing. + + Android's `apply_us` includes a thread handoff and up to one vsync of queue + latency that iOS's does not; it is a before-and-after tool for one platform, + not a cross-platform comparison. **Requires an app generated by `mob_new` + 0.4.32 or newer** — the buffer lives in the app's own `MobBridge.kt`. + +### Fixed +- `Mob.RenderStats.native_*` now report `{:error, :unsupported}` when the + running app's bridge lacks the method, instead of leaking + `{:error, :not_loaded}` past a `@spec` promising two shapes. Android signals + a missing bridge method by returning rather than raising, so it took the one + path that was not converted — which every app generated before this release + hits. + - **`Mob.Test.capabilities/1`** — ask a build which test-harness probes it can actually serve, before choosing how to drive it. Which probes work is a runtime fact: on Android each harness NIF bails when the app's generated diff --git a/android/jni/mob_nif.zig b/android/jni/mob_nif.zig index d85c98a..281d16c 100644 --- a/android/jni/mob_nif.zig +++ b/android/jni/mob_nif.zig @@ -287,6 +287,8 @@ pub const BridgeMethods = extern struct { type_text: jni.JMethodID = null, delete_backward: jni.JMethodID = null, clear_text: jni.JMethodID = null, + render_stats: jni.JMethodID = null, + render_stats_enable: jni.JMethodID = null, long_press_xy: jni.JMethodID = null, swipe_xy: jni.JMethodID = null, screenshot: jni.JMethodID = null, @@ -675,6 +677,7 @@ export fn nif_capabilities( erts.atom(env, "ax_action"), erts.atom(env, "element_frames"), erts.atom(env, "scroll_info"), erts.atom(env, "scroll_to"), erts.atom(env, "sample_region"), erts.atom(env, "screenshot"), + erts.atom(env, "native_stats"), }; const vals = [_]erts.ERL_NIF_TERM{ boolAtom(env, Bridge.ui_view_tree != null), @@ -702,6 +705,10 @@ export fn nif_capabilities( // See decisions/2026-08-10-sample-region-crops-natively-and-stays-debug-only.md boolAtom(env, false), boolAtom(env, Bridge.screenshot != null), + // Both halves come from the same generated bridge, so one flag answers + // for the pair; renderStatsEnable without renderStats would be an app + // that half-applied a template update. + boolAtom(env, Bridge.render_stats != null), }; return erts.makeMap(env, &keys, &vals) orelse erts.atom(env, "error"); } @@ -4241,11 +4248,84 @@ fn nifLoad(env: ?*erts.ErlNifEnv, priv: *?*anyopaque, info: erts.ERL_NIF_TERM) c cacheOptional(jenv, "clearText", "()Z", &Bridge.clear_text); cacheOptional(jenv, "longPressXy", "(FFJ)Z", &Bridge.long_press_xy); cacheOptional(jenv, "swipeXy", "(FFFF)Z", &Bridge.swipe_xy); + cacheOptional(jenv, "renderStats", "()Ljava/lang/String;", &Bridge.render_stats); + cacheOptional(jenv, "renderStatsEnable", "(Z)Z", &Bridge.render_stats_enable); logi_nif("Mob NIF loaded (Compose backend)", .{}); return 0; } +// nif_native_stats/0 — JSON of the recorded native frame samples, newest +// first, matching the shape iOS emits so `Mob.RenderStats` needs no +// per-platform parsing: +// +// {"enabled":bool,"recorded":N,"dropped":M, +// "samples":[{"apply_us":f,"transition":s,"seq":n},...]} +// +// The ring buffer lives in Kotlin rather than here, unlike iOS where it sits in +// C beside the NIF. On Android the measurement can only be taken on the main +// thread — it brackets a Compose frame — so keeping the buffer next to the +// thing that writes it avoids a JNI hop per sample on the hot path. This +// mirrors elementFrames, which is built in Kotlin for the same reason. +export fn nif_native_stats( + env: ?*erts.ErlNifEnv, + argc: c_int, + argv: [*]const erts.ERL_NIF_TERM, +) callconv(.c) erts.ERL_NIF_TERM { + _ = argc; + _ = argv; + if (Bridge.render_stats == null) return notLoaded(env); + var attached: c_int = 0; + const jenv = get_jenv(&attached) orelse return erts.atom(env, "error"); + const jresult = jenv.*.CallStaticObjectMethod.?(jenv, Bridge.cls, Bridge.render_stats); + const result = jstringToBin(env, jenv, jresult); + detachIfAttached(attached); + return result; +} + +// nif_native_stats_enable/1 — turn native frame timing on or off. +// +// Off by default: an enabled measurement arms an idle handler per set_root and +// takes two timestamps, which is not free on a screen that re-renders steadily. +// Enabling also clears the buffer, so a caller measures the run it just started +// rather than whatever was left over from the last one. +export fn nif_native_stats_enable( + env: ?*erts.ErlNifEnv, + argc: c_int, + argv: [*]const erts.ERL_NIF_TERM, +) callconv(.c) erts.ERL_NIF_TERM { + _ = argc; + if (Bridge.render_stats_enable == null) return notLoaded(env); + + var on_buf: [8]u8 = @splat(0); + if (erts.enif_get_atom(env, argv[0], &on_buf, on_buf.len, erts.ERL_NIF_LATIN1) == 0) + return erts.badarg(env); + + // Exact compare against the NUL-terminated atom, not a prefix: `on_buf` is + // 8 bytes, so a prefix test on the first four accepts `:truthy` and + // `:true_x` as "on" — the very typo-enables-it case this refuses. Anything + // that is neither `true` nor `false` is a caller error rather than a + // silent no-op, matching what iOS does. + const on_atom = std.mem.sliceTo(&on_buf, 0); + const on: bool = if (std.mem.eql(u8, on_atom, "true")) + true + else if (std.mem.eql(u8, on_atom, "false")) + false + else + return erts.badarg(env); + + var attached: c_int = 0; + const jenv = get_jenv(&attached) orelse return erts.atom(env, "error"); + _ = jenv.*.CallStaticBooleanMethod.?( + jenv, + Bridge.cls, + Bridge.render_stats_enable, + @as(jni.JBoolean, if (on) 1 else 0), + ); + detachIfAttached(attached); + return erts.ok(env); +} + // ── NIF table + ERL_NIF_INIT entry point ───────────────────────────────── // Replaces the static `ErlNifFunc nif_funcs[]` + `ERL_NIF_INIT` macro // that used to live at the bottom of mob_nif.c. The entry point is the @@ -4270,6 +4350,8 @@ const nif_funcs = [_]erts.ErlNifFunc{ .{ .name = "clear_text", .arity = 0, .fptr = nif_clear_text, .flags = 0 }, .{ .name = "long_press_xy", .arity = 3, .fptr = nif_long_press_xy, .flags = 0 }, .{ .name = "swipe_xy", .arity = 4, .fptr = nif_swipe_xy, .flags = 0 }, + .{ .name = "native_stats", .arity = 0, .fptr = nif_native_stats, .flags = erts.ERL_NIF_DIRTY_JOB_CPU_BOUND }, + .{ .name = "native_stats_enable", .arity = 1, .fptr = nif_native_stats_enable, .flags = 0 }, .{ .name = "screenshot", .arity = 3, .fptr = nif_screenshot, .flags = erts.ERL_NIF_DIRTY_JOB_CPU_BOUND }, .{ .name = "scroll_info", .arity = 1, .fptr = nif_scroll_info, .flags = 0 }, .{ .name = "scroll_to", .arity = 3, .fptr = nif_scroll_to, .flags = 0 }, diff --git a/decisions/2026-09-05-measure-the-native-half-on-android.md b/decisions/2026-09-05-measure-the-native-half-on-android.md new file mode 100644 index 0000000..4bfaecb --- /dev/null +++ b/decisions/2026-09-05-measure-the-native-half-on-android.md @@ -0,0 +1,92 @@ +# Measuring the native half of a frame on Android + +Date: 2026-09-05 +Status: accepted +Ticket: MOB-146 + +## Context + +`decisions/2026-09-03-measure-the-native-half-of-a-frame.md` established the +iOS side: `Mob.RenderStats` can time the BEAM half of a render but not the +native half, and on a dense screen the native half is most of the cost. +Android had none of it — `native_summary/1` returned `{:error, :unsupported}` +there, which is indistinguishable from "you forgot to enable it". + +That made MOB-146 unarguable in the direction it needed to be argued. The +claim is that Android navigation disposes and recreates the composition, the +same defect MOB-129 fixed on iOS. Without a number, a fix for it lands with no +before and no after. + +## Decision + +### The ring buffer lives in Kotlin, not in the NIF + +iOS keeps it in C beside the NIF. On Android the measurement can only be taken +on the main thread, so keeping the buffer next to the writer avoids a JNI hop +per sample on a hot path. `nif_native_stats` fetches the serialised JSON the +way `nif_element_frames` fetches frames, and both Kotlin methods are looked up +with `cacheOptional` so an app generated before they existed still loads. + +### The closing bracket rides the frame + +This is the part that fails silently with a plausible number, so the rejected +options are worth recording. + +`MessageQueue.IdleHandler` is the literal analogue of the +`CFRunLoopObserver(.beforeWaiting)` iOS uses, and it is wrong here. Compose +requests its frame through `Choreographer`, and a vsync callback arrives +asynchronously rather than sitting in the queue. Between the request and the +vsync the queue is genuinely empty, so the handler fires there — before any of +the work being measured — and reports the cost of a field write. + +Posting a plain `Runnable` and registering the frame callback from inside it +fails differently and less visibly. `ViewRootImpl.scheduleTraversals` installs +a sync barrier that blocks non-async messages until `doTraversal` runs. With a +traversal already pending, which is the steady state on exactly the busy +screens being measured, that post is held while Compose recomposes at frame V, +so it registers for V+1 and the sample absorbs a whole extra frame. Upward +bias, bimodal, worst under load. Choreographer's own vsync messages are +asynchronous and sail past the barrier, which is why registering directly from +the calling thread does not have the problem. + +So the frame callback is registered straight from the NIF thread against a +`Choreographer` captured on the main thread at init. It fires at the start of +the next frame; a message posted from inside it cannot run until the traversal +has measured, laid out and drawn, because that traversal is synchronous. + +### Android's `apply_us` is not iOS's `apply_us` + +Recorded plainly because a differential test (MOB-157) would otherwise compare +them and conclude something false. + +iOS starts its clock on the main thread, after the dispatch hop, with the node +already parsed. Android starts on the NIF thread, after `MobJson.parseNode` +but before the state write, and the interval therefore includes a thread +handoff and up to one vsync of queue latency that iOS's does not. + +The parse is deliberately excluded on both. On Android `setRootJson` runs +synchronously from the NIF, so the BEAM-side `set_root_us` already spans the +parse; measuring it here too would double-count it against anyone adding the +two windows. + +What this measurement is for is a before-and-after on one platform, taken the +same way on both sides. It is not an absolute native frame cost, and it is not +comparable across platforms. + +## Consequences + +- First Android navigation baseline, physical moto g power, 1600-node screen: + `none` p50 266ms, `push` p50 841ms, `pop` p50 916ms. Navigation costs 3.2x a + re-render of the same tree, which is the gap MOB-146 exists to close. +- The instrument was corroborated before its numbers were believed: in the + same window the platform logged `Davey! duration=1414ms` and + `Choreographer: Skipped 67 frames` (~1120ms). The measured figures sit just + below the platform's, which is the right direction, since the bracket closes + after the traversal but before GPU swap. +- A burst of `setRootJson` calls arms several brackets that all close on the + same frame, so each attributes the cost of rendering the last tree to trees + that were superseded. Over-counting rather than losing samples, and the same + behaviour iOS has. +- `native_disable/0` keeps the window readable, matching iOS. Clearing on + disable would empty the buffer the caller is about to read, and the result + would look exactly like the feature being off. diff --git a/ios/mob_nif.m b/ios/mob_nif.m index 52a7753..b848fb9 100644 --- a/ios/mob_nif.m +++ b/ios/mob_nif.m @@ -7102,6 +7102,9 @@ static ERL_NIF_TERM nif_capabilities(ErlNifEnv *env, int argc, const ERL_NIF_TER {"clear_text", harness}, {"ax_action", harness}, {"element_frames", harness}, {"scroll_info", harness}, {"scroll_to", harness}, {"sample_region", harness}, {"screenshot", screenshot}, + // Frame timing is part of the harness, so it lives or dies with it — + // compiled out of release builds along with everything else here. + {"native_stats", harness}, }; for (size_t i = 0; i < sizeof(caps) / sizeof(caps[0]); i++) { diff --git a/lib/mob/render_stats.ex b/lib/mob/render_stats.ex index e733d08..715cc3b 100644 --- a/lib/mob/render_stats.ex +++ b/lib/mob/render_stats.ex @@ -564,11 +564,13 @@ defmodule Mob.RenderStats do `set_root`; the timestamps and the run loop observer are downstream of that check. - Returns `{:error, :unsupported}` in three cases, all of which look identical + Returns `{:error, :unsupported}` in four cases, all of which look identical to a caller: on the host, where there is no native side at all; on a platform - whose native half has not implemented it, which today means Android; and in - an **iOS release build**, because the reading NIFs sit inside the same - `MOB_RELEASE` guard as the rest of the test harness. Profile a debug build. + whose native half has not implemented it; in an **iOS release build**, + because the reading NIFs sit inside the same `MOB_RELEASE` guard as the rest + of the test harness (profile a debug build); and on an Android app whose + generated `MobBridge.kt` predates the frame-timing methods — `MobBridge.kt` + is generated once and never re-rendered, so regenerate the app. """ @spec native_enable(module()) :: :ok | {:error, :unsupported} def native_enable(nif \\ :mob_nif), do: native_call(nif, :native_stats_enable, [true]) @@ -655,7 +657,17 @@ defmodule Mob.RenderStats do # whose native half lacks this function). Both mean the same thing to a # caller, and neither should take down whatever is reading stats. defp native_call(nif, fun, args) do - apply(nif, fun, args) + case apply(nif, fun, args) do + # Android reports a missing bridge method by RETURNING this rather than + # raising: the NIF is present in the loaded library, so nothing raises, + # but the Kotlin half is absent because `MobBridge.kt` is generated once + # and never re-rendered. Every app generated before the frame-timing + # methods existed reaches here. Same meaning as the raised cases below — + # this build cannot serve the call — so it gets the same answer, instead + # of leaking a fourth shape past a @spec that promises two. + {:error, :not_loaded} -> {:error, :unsupported} + other -> other + end rescue e in UndefinedFunctionError -> # Only this module's own absence. A different UndefinedFunctionError diff --git a/lib/mob/test.ex b/lib/mob/test.ex index 1f70e3e..4dc120b 100644 --- a/lib/mob/test.ex +++ b/lib/mob/test.ex @@ -489,7 +489,8 @@ defmodule Mob.Test do :scroll_info, :scroll_to, :sample_region, - :screenshot + :screenshot, + :native_stats ] @doc false diff --git a/test/mob/android_native_stats_test.exs b/test/mob/android_native_stats_test.exs new file mode 100644 index 0000000..30ca001 --- /dev/null +++ b/test/mob/android_native_stats_test.exs @@ -0,0 +1,80 @@ +# Source-contract test: the Android NIF table and its capability map are native +# structures Elixir cannot execute. Guards MOB-146. +# credo:disable-for-this-file Jump.CredoChecks.VacuousTest +defmodule Mob.AndroidNativeStatsTest do + use ExUnit.Case, async: true + + @root Path.expand("../..", __DIR__) + + setup_all do + {:ok, zig: File.read!(Path.join(@root, "android/jni/mob_nif.zig"))} + end + + test "native frame timing is registered as a NIF", %{zig: zig} do + # Without these Mob.RenderStats.native_summary/1 returns {:error, :unsupported} + # on Android, which is what it did before this ticket — and it looks + # identical to "the feature is off", which is why it went unnoticed. + assert zig =~ ~s(.name = "native_stats", .arity = 0) + assert zig =~ ~s(.name = "native_stats_enable", .arity = 1) + end + + test "the Kotlin methods behind them are looked up optionally", %{zig: zig} do + # cacheOptional, not cacheRequired: an app generated before this ticket has + # no renderStats method, and must keep loading rather than failing at boot + # with every other NIF. + assert zig =~ ~s|cacheOptional(jenv, "renderStats", "()Ljava/lang/String;"| + assert zig =~ ~s|cacheOptional(jenv, "renderStatsEnable", "(Z)Z"| + end + + test "capability keys and values stay positionally aligned", %{zig: zig} do + # These are two parallel arrays zipped into a map by position. Getting them + # out of step does not fail to compile — the map then reports one NIF's + # availability under another's name. + # + # Counting is not enough to catch that: inserting a key mid-array while + # appending its value at the end keeps the counts equal and produces + # exactly the misordering. So this pairs them up and checks the pairing. + body = capabilities_body(zig) + keys = Regex.scan(~r/erts\.atom\(env, "(\w+)"\)/, section(body, "keys")) + vals = Regex.scan(~r/boolAtom\(env, ([^)]*)\)/, section(body, "vals")) + + assert length(keys) == length(vals), + "capabilities has #{length(keys)} keys and #{length(vals)} values" + + pairs = + Enum.zip( + Enum.map(keys, fn [_, name] -> name end), + Enum.map(vals, fn [_, expr] -> String.trim(expr) end) + ) + + # Every key whose value names a Bridge handle must name the handle for + # THAT key. `native_stats` is served by `render_stats`, and the two + # deliberate `false` literals (ax_action, sample_region) are exempt because + # no bridge method backs them at all. + aliases = %{"native_stats" => "render_stats", "view_tree" => "ui_view_tree"} + + for {key, expr} <- pairs, String.starts_with?(expr, "Bridge.") do + expected = Map.get(aliases, key, key) + + assert expr == "Bridge.#{expected} != null", + "capability #{inspect(key)} reports #{expr}, which belongs to a " <> + "different NIF — the arrays are out of step from here on" + end + + assert {"native_stats", "Bridge.render_stats != null"} in pairs, + "native_stats is registered but not reported by capabilities/1, so " <> + "an agent cannot discover it without calling it and catching the error" + end + + defp capabilities_body(zig) do + [_, body] = String.split(zig, "export fn nif_capabilities", parts: 2) + [body | _] = String.split(body, "\nexport fn ", parts: 2) + body + end + + defp section(body, name) do + [_, rest] = String.split(body, "const #{name} = [_]erts.ERL_NIF_TERM{", parts: 2) + [section | _] = String.split(rest, " };", parts: 2) + section + end +end diff --git a/test/mob/native_frame_stats_test.exs b/test/mob/native_frame_stats_test.exs index e9ab440..f8590e3 100644 --- a/test/mob/native_frame_stats_test.exs +++ b/test/mob/native_frame_stats_test.exs @@ -48,8 +48,8 @@ defmodule Mob.NativeFrameStatsTest do describe "graceful degradation" do test "a native half that has not implemented it reports :unsupported" do - # Android has no native_stats yet, so :mob_nif keeps the Erlang stub and - # it raises. Reading stats must not take down whatever is reading them. + # A platform whose native half lacks the function keeps the Erlang stub, + # and it raises. Reading stats must not take down whatever is reading them. assert {:error, :unsupported} = RenderStats.native_enable(NotLoadedNif) assert {:error, :unsupported} = RenderStats.native_disable(NotLoadedNif) assert {:error, :unsupported} = RenderStats.native_frames(NotLoadedNif) @@ -342,4 +342,76 @@ defmodule Mob.NativeFrameStatsTest do :nomatch -> flunk("expected to find #{inspect(needle)}") end end + + describe "Android-shaped payloads" do + # Android builds this JSON by hand in Kotlin (a StringBuilder in + # MobBridge.kt) rather than through a serialiser the way iOS does with + # NSJSONSerialization. Hand-built JSON is exactly the kind that is + # well-formed for the values you tried and malformed for the one you did + # not, so the shape it actually emits is pinned here. + + defmodule AndroidNif do + @moduledoc false + # A wrapped ring: 250 frames recorded, 240 retained, 10 scrolled off. + def native_stats do + samples = + for seq <- 249..10//-1 do + ~s({"apply_us":#{seq}.5,"transition":"push","seq":#{seq}}) + end + + ~s({"enabled":true,"recorded":250,"dropped":10,"samples":[) <> + Enum.join(samples, ",") <> "]}" + end + + def native_stats_enable(_on), do: :ok + end + + defmodule QuotedTransitionNif do + @moduledoc false + # `transition` reaches the buffer from nif_set_transition, which accepts + # any atom up to 15 characters verbatim. An unescaped quote here costs + # the reader the whole window, not the one sample. + def native_stats do + ~s({"enabled":true,"recorded":1,"dropped":0,) <> + ~s("samples":[{"apply_us":1.0,"transition":"a\\"b","seq":0}]}) + end + + def native_stats_enable(_on), do: :ok + end + + test "a wrapped ring reports retained, recorded and dropped consistently" do + summary = RenderStats.native_summary(AndroidNif) + + assert summary.samples == 240 + assert summary.recorded == 250 + assert summary.dropped == 10 + + # The identity that makes `dropped` meaningful: what you can still read, + # plus what scrolled away, is everything that happened. + assert summary.samples + summary.dropped == summary.recorded + end + + test "an escaped quote in a transition still parses" do + summary = RenderStats.native_summary(QuotedTransitionNif) + + assert summary.samples == 1 + assert Map.has_key?(summary.apply_us, ~s(a"b)) + end + + test "an UNescaped quote loses the whole window, not one sample" do + # Pinning the consequence, so the escaping in MobBridge.kt is understood + # as load-bearing rather than defensive tidiness. + defmodule BrokenNif do + @moduledoc false + def native_stats do + ~s({"enabled":true,"recorded":1,"dropped":0,) <> + ~s("samples":[{"apply_us":1.0,"transition":"a"b","seq":0}]}) + end + + def native_stats_enable(_on), do: :ok + end + + assert {:error, _} = RenderStats.native_frames(BrokenNif) + end + end end From 1e4a1a1d4b59bd715a34fa5ba03589101c76ed61 Mon Sep 17 00:00:00 2001 From: GenericJam Date: Sat, 5 Sep 2026 14:32:03 -0600 Subject: [PATCH 2/4] Reflow caps[] for clang-format Adding native_stats on its own line changed the column alignment of the whole block, so clang-format wanted to reflow it to one entry per line. CI runs `clang-format --dry-run -Werror` over ios/mob_nif.m; the repo's pre-commit checklist names `mix format` and credo but not the native formatters, which is how this reached CI. Co-Authored-By: Claude Opus 5 (1M context) --- ios/mob_nif.m | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/ios/mob_nif.m b/ios/mob_nif.m index b848fb9..1e26bf3 100644 --- a/ios/mob_nif.m +++ b/ios/mob_nif.m @@ -7096,11 +7096,21 @@ static ERL_NIF_TERM nif_capabilities(ErlNifEnv *env, int argc, const ERL_NIF_TER const char *name; int on; } caps[] = { - {"view_tree", harness}, {"ui_tree", harness}, {"screen_info", harness}, - {"tap_xy", harness}, {"tap_by_label", harness}, {"long_press_xy", harness}, - {"swipe_xy", harness}, {"type_text", harness}, {"delete_backward", harness}, - {"clear_text", harness}, {"ax_action", harness}, {"element_frames", harness}, - {"scroll_info", harness}, {"scroll_to", harness}, {"sample_region", harness}, + {"view_tree", harness}, + {"ui_tree", harness}, + {"screen_info", harness}, + {"tap_xy", harness}, + {"tap_by_label", harness}, + {"long_press_xy", harness}, + {"swipe_xy", harness}, + {"type_text", harness}, + {"delete_backward", harness}, + {"clear_text", harness}, + {"ax_action", harness}, + {"element_frames", harness}, + {"scroll_info", harness}, + {"scroll_to", harness}, + {"sample_region", harness}, {"screenshot", screenshot}, // Frame timing is part of the harness, so it lives or dies with it — // compiled out of release builds along with everything else here. From 678db269b82b77b5705fa1033fae5c1998aaf81f Mon Sep 17 00:00:00 2001 From: GenericJam Date: Sat, 5 Sep 2026 14:35:04 -0600 Subject: [PATCH 3/4] Add the native formatters to the pre-commit checklist MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Item 5 said `mix format && mix credo --strict`, and mix format says nothing about ObjC. CI runs clang-format over ios/mob_nif.m with -Werror, so adding one entry to an aligned C initialiser fails the build — clang-format re-flows the whole block around it. That is exactly what happened on this branch. Co-Authored-By: Claude Opus 5 (1M context) --- AGENTS.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index 0efb9af..8953852 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -175,6 +175,14 @@ These are the things we've burned ourselves on. Following them isn't optional. relevant repo, every time. Both are clean across the codebase today; don't regress them. + **And the native formatters, if you touched native source.** CI runs + `xcrun clang-format --dry-run -Werror ios/mob_nif.m android/jni/mob_beam.h` + and swiftlint, and `mix format` says nothing about either. Adding one line + to an aligned C initialiser is enough to fail it, because clang-format + re-flows the whole block around the new entry — which is how this note came + to be written. Run `xcrun clang-format -i ` on any `.m`/`.h` you + edited before committing. + 6. **Multi-repo changes batch together.** A user-visible fix in mob often needs matching changes in mob_dev (build) and mob_new (template). Bumping versions without coordination produces ghost regressions. Check all three before From 620c6a09efcfc2c032620c0b1b801534acc3aced Mon Sep 17 00:00:00 2001 From: GenericJam Date: Sat, 5 Sep 2026 19:52:52 -0600 Subject: [PATCH 4/4] Test the not_loaded conversion, and correct the capabilities guide MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two gaps found in pre-merge review. The headline fix of this PR was untested. `native_call/3` converting a RETURNED `{:error, :not_loaded}` to `:unsupported` is the path every pre-existing Android app takes — the NIF is present so nothing raises, but the Kotlin half is absent because MobBridge.kt is generated once and never re-rendered. Deleting that clause left all 1545 tests green: every stub in the file either raises or returns valid JSON, so nothing exercised a NIF that returns the tuple. Mutation-checked now — removing the clause fails the new test. guides/agentic_coding.md claimed "every one of the seventeen keys is shown, because guessing at the rest is exactly what goes wrong", which this PR made false by adding an eighteenth. The example map is now complete again, labelled as the pre-0.4.32 template it actually describes, and says what regenerating flips — including that clear_text stays false on purpose. The guide arguing against guessing at capabilities should not itself be a stale table. Co-Authored-By: Claude Opus 5 (1M context) --- guides/agentic_coding.md | 20 ++++++++++++++------ test/mob/native_frame_stats_test.exs | 28 ++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 6 deletions(-) diff --git a/guides/agentic_coding.md b/guides/agentic_coding.md index 1a6db37..f696a89 100644 --- a/guides/agentic_coding.md +++ b/guides/agentic_coding.md @@ -192,15 +192,23 @@ Mob.Test.capabilities(node) #=> swipe_xy: false, type_text: false, delete_backward: false, #=> clear_text: false, ax_action: false, element_frames: true, #=> scroll_info: true, scroll_to: true, sample_region: false, -#=> screenshot: true +#=> screenshot: true, native_stats: false #=> } ``` -That is a **freshly generated Android app** — abridged only in layout, not in -content; every one of the seventeen keys is shown, because guessing at the rest -is exactly what goes wrong. The template defines `screenInfo`, `elementFrames`, -`screenshot`, `scrollInfo` and `scrollTo`, and nothing else in the harness set -(MOB-160). +That is an Android app generated **before `mob_new` 0.4.32** — abridged only in +layout, not in content; every one of the eighteen keys is shown, because +guessing at the rest is exactly what goes wrong. That template defined +`screenInfo`, `elementFrames`, `screenshot`, `scrollInfo` and `scrollTo`, and +nothing else in the harness set. + +Regenerating against 0.4.32 or newer flips `tap_xy`, `long_press_xy`, +`swipe_xy`, `type_text` and `delete_backward` to `true` (MOB-160) and +`native_stats` to `true` (MOB-146). `clear_text` stays `false` there on +purpose — two implementations of it reported success while clearing nothing, +so the bridge ships without one rather than lie. Which is the point of asking +the build instead of reading a table: this paragraph is already a snapshot of +two releases, and `capabilities/1` is not. Two of those `false`s bite harder than they look: diff --git a/test/mob/native_frame_stats_test.exs b/test/mob/native_frame_stats_test.exs index f8590e3..23e6c04 100644 --- a/test/mob/native_frame_stats_test.exs +++ b/test/mob/native_frame_stats_test.exs @@ -343,6 +343,34 @@ defmodule Mob.NativeFrameStatsTest do end end + describe "a bridge method the app does not have" do + # Android reports a missing bridge method by RETURNING {:error, :not_loaded} + # rather than raising: the NIF is present in the loaded library, so nothing + # raises, but the Kotlin half is absent because MobBridge.kt is generated + # once and never re-rendered. Every app generated before the frame-timing + # methods existed takes this path — which is to say, the common case. + # + # It went untested when it was written, and the omission was invisible: + # deleting the conversion in native_call/3 left the entire suite green, + # because every other stub here either raises or returns valid JSON. + defmodule ReturnsNotLoadedNif do + @moduledoc false + def native_stats, do: {:error, :not_loaded} + def native_stats_enable(_on), do: {:error, :not_loaded} + end + + test "reports :unsupported, not the raw :not_loaded" do + # The @spec promises :ok | {:error, :unsupported}. Leaking a third shape + # breaks every caller matching on :unsupported, and it looks like a + # working NIF returning an error rather than a build that cannot serve + # the call at all. + assert {:error, :unsupported} = RenderStats.native_enable(ReturnsNotLoadedNif) + assert {:error, :unsupported} = RenderStats.native_disable(ReturnsNotLoadedNif) + assert {:error, :unsupported} = RenderStats.native_frames(ReturnsNotLoadedNif) + assert {:error, :unsupported} = RenderStats.native_summary(ReturnsNotLoadedNif) + end + end + describe "Android-shaped payloads" do # Android builds this JSON by hand in Kotlin (a StringBuilder in # MobBridge.kt) rather than through a serialiser the way iOS does with