MOB-102: fix element_frames staleness after a no-op-frame rerender - #87
Merged
Conversation
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
nif_set_root wiped the whole frame registry on every render, relying on MobFrameTracker's onChange(of: frame) to repopulate every surviving element -- but that onChange only refires when the frame VALUE changes, so a static, unmoved element stayed wiped until something eventually moved it. Mob.Test/element_frames would report a still-visible, unmoved element as missing. Fix: purge by id instead of wiping everything. nif_set_root now parses the incoming tree first, collects its live :id set, and removes only registry entries absent from that set -- a surviving element's entry is never touched, so there's nothing to race and nothing that needs to "repopulate." MobRootView.swift is unchanged. Three "make every surviving element re-register on every render" variations were tried and rejected first (ObservedObject-keyed onChange, Environment-keyed onChange, forced .id() reconstruction) -- all shared the same failure, confirmed via an NSLog-instrumented device build: an @Published/@Environment change reaches a view mid- removal one more time before SwiftUI prunes it, so a removed element's stale frame survived its own removal. See decisions/2026-08-27-frame-registry-purge-by-id.md for the full device-verification trail. Device-verified: iOS 17 Pro simulator, a throwaway two-element repro screen (one static, one removed by a later tap) driven via Mob.Test- equivalent RPC. No XCTest target exists in this repo for native iOS changes (see CLAUDE.md) -- this is documented manual verification, not CI-enforced. Co-Authored-By: Claude Sonnet 5 <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 27, 2026
* MOB-103: element_frames liveness — tree membership isn't rendered MOB-102 (0.7.29) replaced nif_set_root's unconditional mob_clear_frames() with a purge-by-id. That fixed static elements vanishing, but swapped the predicate: element_frames/1 documents "rendered on screen" while purge-by-id implements "present in the BEAM tree". Two gaps, both from PR #87 review. 1. In the tree but not laid out. LazyVStack discards far-offscreen rows, TabView keeps every tab's subtree in the tree while showing one, and a dismissed sheet's content stays mounted. Their ids survive the purge and onChange never fires again, so the last on-screen frame was reported forever — tap_id then taps whatever occupies those coordinates now. Before MOB-102 the wipe made this {:error, :not_found}; MOB-102 turned a safe failure into a silently wrong action. 2. The teardown race is narrowed, not closed. mob_adopt_frame_ids runs synchronously on the NIF thread, but MobViewModel.setRoot is dispatched async to main — so an outgoing screen is still animating out under .transition(navTransition(...)) after its ids were purged, and its GeometryReader re-registers it at mid-animation coordinates. Fix, keeping purge-by-id: - mob_register_frame returns a monotonic write seq; MobFrameTracker holds it in @State and hands it back on .onDisappear via mob_unregister_frame(id, seq). Compare-and-delete, so an outgoing screen can't remove an entry an incoming screen just claimed under the same :id. Deliberately on the unregistration side — all four mechanisms MOB-102 tried and rejected were attempts to make registration race-proof against removal timing. - nif_set_root retains the live id set; writes for ids absent from it are ignored, killing the mid-animation re-registration for non-shared ids. - Seqs live in a side table so nif_element_frames keeps JSON-encoding the registry directly — the {"id":[x,y,w,h]} wire shape is unchanged. Also: document what counts as rendered on element_frames/1 (the docstring still promised "every rendered element") plus the settle caveat, and add the MOB-102 entry the 0.7.29 changelog never got despite the behaviour being user-visible. Verified: ios/mob_nif.m compiles clean (clang -fsyntax-only -fobjc-arc, iOS 26.5 sim SDK); registry algorithm covered by an executable harness asserting all 12 behaviours including the MOB-102 non-regression case; 1111 Elixir tests, format and credo clean. NOT verified: whether .onDisappear fires for a LazyVStack row leaving the render window / a TabView tab switching away — needs a device run, no XCTest target in this repo. No version bump — CHANGELOG entry is under [Unreleased]. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * MOB-103: address adversarial review — two provable defects + overclaimed docs An adversarial review of the first commit found two source-provable bugs in the SwiftUI half, both of which reintroduced the very failure MOB-102 exists to prevent (a still-visible element reported missing). Verified both against the source before acting. 1. Trackers aren't bound to an :id. Every ForEach in MobRootView.swift keys children by index (id: \.offset) while the registry is keyed by :id, so deleting a list item shifts each later id onto a tracker that keeps its view identity. With same-height rows the tracker's own frame value doesn't change, so nothing fired — and the departing tracker's .onDisappear compare-and-deleted the SURVIVING element's entry, which then never re-registered. Broken on the commonest list operation. Fixed by re-registering on .onChange(of: id). Order-independent: whichever of the teardown and the re-registration lands first, the survivor ends up correctly registered at its new position. Keying the seven ForEach sites on nativeViewId would make this structural rather than incidental and is the better long-term fix, but it changes SwiftUI identity semantics across every list (@State preservation, animations), so it wants its own change and its own device pass — recorded as a known gap in the decision doc. 2. The shared-:id nav case was stated as a guarantee but wasn't one. navTransition is .move for push/pop, so the outgoing screen's global frames change continuously and it keeps firing onChange as it slides out. When both screens tag an element with the same :id, that id IS in the new tree, so the tree-membership gate doesn't reject it: the outgoing screen re-claims the id after the incoming one, then its .onDisappear deletes it — with the winner decided nondeterministically by intra-frame update order. "Until the surviving element next moves" is never, for a static button. Fixed with generation gating. A non-"none" transition is what bumps navVersion, and MobRootView keys the tree on .id(currentNavVersion), so all identities are rebuilt; nif_set_root bumps a frame generation in lockstep, trackers capture it on appear, and superseded writes are refused. The refused write returns seq 0 and unregister no-ops on 0, so the outgoing teardown can't delete the incoming entry either. Read through a plain C call, not @Environment/@ObservedObject — it's a rejection gate, not a repopulation trigger, which is what separates it from MOB-102's four rejected attempts. Also from the review: - Re-register on .onAppear. Re-registration previously relied on onChange(initial: true) firing again, which is not guaranteed when SwiftUI preserved view identity across the disappearance — a TabView tab demonstrably preserves @State across switches, so a tab could have been permanently unreportable after one round trip. - Move per-tracker bookkeeping off @State scalars into a reference box. The writes happen inside a layout-driven callback once per display frame per element during a transition (the "modifying state during view update" shape), and the seq must stay readable during the teardown transaction that reads it. - nif_element_frames snapshots under the lock and serializes outside it. The main thread takes that lock on every frame write and the docs tell callers to poll this NIF. - mob_nif.m now imports MobDemo-Bridging-Header.h so signature drift is a compile error. C has no name mangling, so before this the void -> uint64_t change was checked by nothing and a future revert would have silently fed Swift a garbage return register. - element_frames/1 docs note the reorder gap rather than promising more than the implementation delivers. - The retroactive MOB-102 bullet in the frozen [0.7.29] section is marked as added after release, so the changelog stays a record of what each release actually shipped. Verification, corrected — the previous commit message overstated this: - test/native/ now holds the harness as a committed, runnable target (make -C test/native run), 20 assertions including both MOB-102 non-regression cases, the shared-:id nav case and the list-delete case. It reproduces the registry functions rather than linking the shipped ones, so it checks the algorithm, not the binary. That limitation is stated in the file. - ios/mob_nif.m compiles clean under clang -fsyntax-only -fobjc-arc against the iOS 26.5 simulator SDK, with MobApp-Swift.h stubbed (it's generated by swiftc during the real build) — the earlier "compiles clean" claim omitted that the stub was required. - clang-format and swiftlint clean (the one force_cast warning is pre-existing on master). The 1111 Elixir tests pass but exercise none of this; the only Elixir change is a docstring. STILL UNVERIFIED, and it is what decides whether this is a fix or a regression: whether .onDisappear fires for a LazyVStack row leaving the render window (and whether LazyVStack destroys such rows at all rather than retaining them), whether a TabView round trip re-registers, and whether app backgrounding fires a spurious .onDisappear that could empty the registry. None is answerable from source and there is no XCTest target here. Needs a simulator pass reading Mob.Test.element_frames/1 over dist at each step. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * MOB-103: record device verification — the headline claim now holds in practice The adversarial review's verdict was that no amount of source reading decides whether this change is a fix or a regression, and it was right. Verified on both an iPhone 17 Pro simulator (iOS 26.4) and a physical iPhone (iOS 26.5.2), using a generated probe app — a 300-row :list plus two screens deliberately tagging a button with the same :id — read over dist with Mob.Test.element_frames/1. Divergence 1, the headline. Only the visible window is ever registered (12 of 300 rows on the simulator, 10 on the phone), so LazyVStack does destroy far-offscreen rows and .onDisappear does fire for them — the load-bearing assumption the review flagged as unverified, and it holds. After scrolling well past it, row_3 is absent on BOTH targets. Under MOB-102 alone that id stays in the tree, so nothing would have dropped it and tap_id would have tapped whatever now occupies those coordinates. Rows scrolled back into range re-register, so the cycle is self-healing. MOB-102 non-regression: probe_title and shared_btn, static and unmoved, survived all of that without re-registering. Divergence 2, shared :id across a nav push — the case the review called nondeterministic. The incoming screen's frame is what's reported (simulator y=512 vs the outgoing y=112; phone y=470.5 vs 70.5), stable across repeated reads seconds after the animation settles, and restored exactly on pop. Neither deleted nor left at mid-animation coordinates. Backgrounding: a background/foreground cycle leaves the registry intact, so the "one cycle could empty the registry" failure mode doesn't occur. Still unverified: MobTabView. A tab switching away keeps its subtree in the tree and TabView preserves view identity across switches, so whether .onAppear re-fires on re-selection is NOT settled by the lazy-list result, which destroys and rebuilds instead. The probe couldn't reach that path — tab_bar/1 navigation didn't render a tab bar in the generated app — so it keeps its own note rather than being quietly folded into the verified set. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * MOB-103: MobTabView verified — the last open scenario closes The one case the lazy-list result could not settle. A tab switching away keeps its subtree in the tree and TabView preserves view identity across switches, so whether .onAppear re-fires on re-selection was genuinely open — the review's worry was that a tab could be permanently unreportable after one round trip. Verified on the simulator with a real tab_bar render node: with tab A active only tab_a_marker is registered; switching to B drops it and registers tab_b_marker; switching back restores tab_a_marker. Repeatable in both directions. .onAppear does re-fire, and the deactivated tab's element is dropped rather than left stale. Two findings worth recording while getting there: - The reachable path is the tab_bar RENDER NODE (props.tabs + props.active), not Mob.App.tab_bar/1 navigation — that helper did not render a tab bar in the generated app. - Mob.Renderer has no on_tab_select handler, so that prop reaches native as a raw {pid, tag} tuple and kills the screen process on serialize. Tab selection has to be driven from Elixir through `active`. Separate bug, not this change's; noted in the decision doc. MobTabView is the only scenario left simulator-only: the phone had rebound dist to its USB link-local address (169.254.1.100), which dist can't route to, and clearing that needs the cable physically unplugged. Every other scenario ran on both targets and agreed exactly, and the mechanism under test is SwiftUI behaviour rather than anything device-specific. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Summary
nif_set_rootwiped the wholeelement_framesregistry on every render and relied onMobFrameTracker'sonChange(of: frame)to repopulate every surviving element — but that only refires on a genuine value change, so a static, unmoved element stayed wiped until something eventually moved it.nif_set_rootparses the incoming tree first, collects its live:idset, and removes only registry entries absent from that set. A surviving element's entry is never touched.MobRootView.swiftis unchanged — the whole fix lives inios/mob_nif.m.decisions/2026-08-27-frame-registry-purge-by-id.md.Test plan
mix test— 1111 passedmix format --check-formatted/mix credo --strict/mix compile --warnings-as-errorscleanxcrun clang-format --dry-run -Werror ios/mob_nif.mcleanxcrun simctl spawn <udid> log stream— confirmed: a static element survives an unrelated sibling's rerender, a removed element's frame disappears on the same render that removes it. No XCTest target exists in this repo for native iOS changes; this is documented manual verification perCLAUDE.md.Linear: MOB-102
🤖 Generated with Claude Code