Page curl: one native state machine for the bake loop

[artemlitch]

Problem

A settle that arrived while a bake was running superseded it. The running cycle abandoned its steps at the next generation check, the neighbor slot it was filling stayed blank, and the next swipe onto that side curled onto blank paper. The superseding settle was usually programmatic: an image finishing load fires invalidate, which posts settled. So the reader saw a flash without doing anything.

Underneath that, five facts were held on both sides of the bridge and kept equal only by messages that arrive late: readiness, who is moving the webview, the resting page, whether the manager is mid-transition, and whether a tap turned a page. Every bug we hit was two of those messages interleaving in an order the flags did not anticipate.

Fix

The controller keeps one state: idle, animating, baking, awaitingSettle, awaitingTapResult.

• A bake runs every step to the end. A settle that arrives during a bake or a turn is kept (last wins) and applied once, after. _cycleGeneration and every guard on it are gone. One _teardownCount, bumped only by teardown, stops steps still in flight from a torn-down controller.
• A landed curl rotates the slots so the cover shows the landed page, then calls the manager's instantlyScrollPageDown or instantlyScrollPageUp, the margin tap's own function. The manager's settle bakes all three slots again. The commit and setReady bridge calls are removed. Native never asserts page identity, and it never computes a page number: it says the direction and the manager handles the chunk edge.
• While the landing's settle is awaited, a settle that names the page just left is dropped; the manager always leaves that page or switches chunk. The landing call failing, or the manager's own unsettled message, ends that window, so a settle for a page the webview is still on is never dropped. A bake that stopped early blanks every slot, goes straight to awaitingSettle and asks for a rebake. After three failed bakes in a row the controller stops asking and goes idle: taps still turn pages, and the next settle bakes again. A 3 second watchdog logs if a landing never gets its settle.
• A clear view above the renderer swallows touches whenever the state is not idle. Whether a touch reached the webview is read from UIKit's hit test at touch-down (touch.view inside the webview). A finger that landed on the blocker is dropped, not waited on.
• The controller calls the manager's own methods on window.s (nativePageCurlJump, nativePageCurlPeek, nativePageCurlUnpeek, invalidateNativePageCurl); there is no separate bridge object to keep in sync.

Measured

iPad mini (A17 Pro) simulator, from the native [page-curl] log:

• Within-chunk turn, landed to idle: about 300 ms (three snapshots at 35 to 55 ms each plus four bridge jumps).
• Chunk-edge turn: 1.0 to 1.2 s. Four manager chunk switches at 170 to 280 ms each make up most of it.
• Five settled messages fired during one bake: four kept, one coalesced rebake, then idle. The old controller produced four abandoned cycles and a blank slot from the same input.
• Tap and swipe spam in five bursts, landscape spread: 157 transitions, 37 bakes, 27 taps answered, 39 touches landing on the blocker with zero state changes, idle after every burst.
• A settle burst across a back turn over a chunk edge: 9 settles for the page just left dropped, the new chunk's settle baked, no flip-back.

Cost

A within-chunk turn snapshots three slots instead of one. That is two extra snapshots of about 40 ms each, behind the cover.

Pairs with readwiseio/rekindled#12935, which removes the bridge object from the manager and pins this branch.

🤖 Generated with Claude Code

[artemlitch]

Convention Audit

Every pattern in the diff compared against the rest of apple/ and the manager on artem/bookwise-metal-page-curl.

Novel patterns introduced

• An NS_ENUM state machine with a name function. The only other enum in apple/ is upstream's typedef enum in RNCWebViewImpl.h:19. NS_ENUM is the right form.
• A touch-swallowing view. apple/ only has pass-through overlays (userInteractionEnabled = NO at RNCWebViewImpl.m:964 and RNCPageCurlRenderer.mm:335). The mechanism is sound: host-level recognizers still see touches hit-tested to the blocker.

Matches

• setState:reason: logging only on change is the same shape as master's markReady:/markNotReady:.
• _epoch is master's _cycleGeneration narrowed to teardown.
• touch.view uses the observer's existing touch property, set before onTouchesBegan fires and cleared before onTouchesEnded, so caching the result at touch-down is the correct order.
• Bridge names, argument shapes and message keys match NativePageCurlBridge in types.ts and the object in HorizontalPaginatedScrollingManager.ts:107-115.

Watch item, not a finding

A margin tap while the manager is already processing scroll events queues the event, so touchEnd reaches native before unsettled. Native goes AwaitingTap -> Idle -> AwaitingSettle, with a brief Idle window where a pan could start on the still-valid bakes. Same shape as master; the outcome is benign because the landed cover is what scrollToPage is asked to match.

[artemlitch]

If I were implementing this

Verdict: the core is right. I'd keep the design and change two things inside it.

One state plus one pending settle is the correct shape for this problem. The old controller kept five booleans in sync with messages that arrive late; every bug was a pair of those messages in an order the flags did not anticipate. Collapsing them to a single enum removes the class, not the instances. Two details make it hold rather than just look tidy:

• The input blocker's visibility is derived from the state in setState: rather than toggled by hand at the call sites. There is no path that forgets it.
• Native no longer asserts page identity. A landed curl asks the manager to scrollToPage like a margin tap would, and the manager's own settle is the only thing that starts a bake. That removes commit and setReady from the bridge and with them the two places the sides could disagree about where the page is.

What I'd change, cheapest first:

1. Drop the stale-settle case (finding on line 981). A settle posted during Animating and delivered after the landing is the one remaining interleave the state machine does not close, and it produces exactly the flash the PR is removing. Comparing the settle's page and chunk to the pre-turn _page/_chunkIndex is a two-line guard that needs nothing from the JS side.
2. Blank the slots on a failed bake (finding on line 969). Master locked on failure; this version opens input with wrong bakes. Blanking makes beginTurn refuse, and a requestRebake: brings the next settle.
3. Fold the two duplicated blocks and delete hideIfIdle. That also moves the last _renderer.hidden hand-toggle for Idle into setState:, so both overlays follow the state the same way.

The load-bearing risk to watch: AwaitingSettle has no exit but a settle. The old model degraded to "taps work, no curl"; this one degrades to "no input at all". I traced the manager paths that could drop a settle (invalidateNativePageCurl returns early while a scroll is in flight; loadNextChunk/loadPreviousChunk return early when already loading) and each is covered by a settle already on its way from the in-flight scroll, so I do not think it hangs today. But nothing enforces that. A logged watchdog on time spent in AwaitingSettle (log only, no recovery) would tell us from Sentry breadcrumbs if a path ever appears, before a reader reports a page that stopped responding.

What is good and should stay: reading touch.view at touch-down rather than waiting for a touchEnd that may never come, refusing to curl toward a slot with no texture, and the measured numbers in the description. The cost stated (two extra snapshots behind the cover) is the right trade for never having a half-baked slot.

[artemlitch]

Code Review

Summary

• 7 issues found
• 5 MEDIUM, 2 LOW
• Posted as COMMENT because this is the author's own PR; the labels are what a REQUEST_CHANGES would carry.

The bridge contract was checked against HorizontalPaginatedScrollingManager.ts on artem/bookwise-metal-page-curl: commit and setReady are gone on both sides, message shapes match, and a past-edge scrollToPage does run the chunk switch and settle.

[artemlitch]

Re-review of a39031c

Resolved

• AwaitingTapResult, _teardownCount; the !_enabled half of the runSteps: check is gone and teardown is the only writer of _enabled, so nothing is lost.
• hideIfIdle deleted; setState: hides the renderer on Idle, so both overlays follow the state.
• finishTurnToward: inlined in the same order with nothing lost.
• Watchdog added; "controller" and "turn cancelled" wording.

Still open (unchanged, no reply)

• MEDIUM: stale settle delivered after the landing (line 1328).
• MEDIUM: failed bake goes Idle with stale neighbor slots (line 970).
• MEDIUM: duplicated pending-or-idle block (965 / 1314) and duplicated wait-for-settle block (558 / 994).
• MEDIUM: comment above callBridge:page:completion: still says commit (778).
• LOW: #pragma mark - bake cycles (809); _edgeEmitted never resets on the not-baked path (1110).

New

• LOW: the watchdog is keyed on landings rather than state entries, so it can log a false positive (see inline).

The two correctness findings are the only things between this and a merge I would stand behind.

[artemlitch]

Re-review of 79a8834

Resolved

Every item from the last round: the stale-settle guard, failed-bake blanking, both duplications (bakePendingOrIdle:, awaitSettleIfResting:), the commit comment, the pragma, _edgeEmitted, and the watchdog on _stateSerial. The serial is the right key: captured after the Animating -> AwaitingSettle transition, bumped by any later transition and by teardown, so no false positive and no miss.

New

• MEDIUM: the stale-settle guard can wedge on the scrollToPage failure path (inline at 1019).
• MEDIUM: the failure path in finishBake: routes through Idle and takes its side effects (inline at 998).
• LOW: no cap on the failed-bake retry (inline at 999).
• LOW: the PR body names awaitingTap and _epoch, which no longer exist; the line counts are from the first commit; the stale-settle guard, failure blanking and watchdog are not mentioned.

Nits: "resting" in awaitSettleIfResting: is not defined anywhere (idle, or waiting on a tap); the finishBake: header comment describes only the failure branch.

[artemlitch]

[MEDIUM] This can wedge. _awaitingLanding is only cleared by a bake or teardown, so once a same-page settle is dropped here, every later same-page settle is dropped too (theme, resize, highlights, re-enable).

A legitimate same-page settle arrives when the scrollToPage bridge call fails (1361): the webview never moved, requestRebake: sends invalidate, the manager settles on the page it is still on, and that settle matches the pre-turn page and chunk. The controller then sits in AwaitingSettle with the cover showing the neighbor's snapshot (slots already rotated at 1351), the webview on the old page, and no input. The watchdog logs it; nothing recovers. Same shape if the JS side re-enables after a reload while the flag is set.

Two parts to the fix: clear _awaitingLanding in the !ok branch at 1361 before requestRebake:, and bound the drop so at most one settle is ever discarded. Either clear it on the first drop here, or clear it on the landing's unsettled (the manager posts it before it moves, and delivery is in order, so anything after it is post-move). The second is exact; the first is one line.

[artemlitch]

Fixed in b3920c3, both parts. The !ok branch clears _awaitingLanding before requestRebake:. The unsettled handler clears it too, so only settles posted before the landing can be dropped. I took the unsettled option over first-drop because a burst of pre-landing settles must all be dropped. Tested on the iPhone Duo simulator by making instantlyScrollPageDown throw: before, the log ended at "settle for the page just left; dropped" and the watchdog fired; after, it goes awaitingSettle -> baking -> idle on the original page.

[artemlitch]

[MEDIUM] Going through Idle to reach AwaitingSettle is observable: it hides the renderer, drops the blocker, emits ready, and runs beginTurnForPan. With a finger down, beginTurn finds the blanked slot, emits a spurious edge (1143), and sets _turnDeclined, which holds until that pan ends even after the rebake succeeds. The hop only exists because awaitSettleIfResting: needs Idle to fire.

[self setState:RNCPageCurlStateAwaitingSettle reason:@"bake failed"] plus the invalidate call goes straight there, and then the pending-settle check at 994 can be dropped in favour of bakePendingOrIdle: semantics or left as the one three-line copy.

[artemlitch]

Fixed in b3920c3. finishBake: sets AwaitingSettle directly and calls invalidateNativePageCurl, so there is no ready emit, no renderer hide and no beginTurnForPan. With nativePageCurlJump made to throw, the log shows baking -> awaitingSettle (bake failed) with no idle in between.

[artemlitch]

[LOW] No cap on the retry: bake fails, invalidate, settle, bake fails again. Most steady snapshot failures also pause rAF (backgrounded, detached screen), so the loop self-limits, but a zero-size webview while parented does not, and layoutWithBounds rebakes on resize so a collapsed layout can enter it. A consecutive-failure cap that stops the self-rebake and waits for the next external settle bounds it.

[artemlitch]

Fixed in b3920c3. _failedBakes counts failures in a row and a good bake or teardown resets it. At 3 (RNCPageCurlMaxFailedBakes) the controller logs, goes idle with blank slots and stops invalidating; taps still turn pages and the next settle from the manager bakes again. With the jump made to throw: three failed bakes in 65 ms, then idle; after restoring it, the next settle baked and turns worked.

[artemlitch]

Re-review of 13a4abb (covers 13cb732 and b3920c3)

Resolved, verified in the code

• Stale-settle wedge: the !ok branch and the unsettled handler both clear _awaitingLanding. The landing's own unsettled always precedes its settle, so this also closes the wedge sub-case of the direction-based landing.
• Failed bake goes Baking -> AwaitingSettle directly: no ready, no spurious edge, no _turnDeclined. The renderer keeps the correct current-page cover or stays hidden; blank paper is never drawn.
• Retry cap: _failedBakes cannot spin. The uncounted pending-settle path sends no invalidate; the counted path stops at three.
• PR body is current.
• window.s bridge: all six method names and argument shapes match the manager on artem/bookwise-metal-page-curl, and the old window.nativePageCurl object and type are fully removed on the JS side.

Open

• MEDIUM: stale references after the window.s rename (inline at 53; also RNCWebViewPageCurl.h:13).
• MEDIUM: landing failures cannot reach the !ok branch (inline at 1376; fix is on the JS side).
• LOW: four items inline.

Verdict

The design is sound and I find no open correctness bug in the native state machine. The void return is the one item I would still fix before merge: it is what makes the failure handling added this round unreachable from the scroll path, and it is a two-word change in the rekindled PR.

[artemlitch]

[MEDIUM] Stale after 13cb732: a landing now calls instantlyScrollPageDown/instantlyScrollPageUp, not scrollToPage. Same in RNCWebViewPageCurl.h line 13, which still says the controller drives the webview through window.nativePageCurl; it is window.s now.

[artemlitch]

Fixed in 42bbed4. The state comment says "a landed curl's page turn" and the header says the controller drives the scrolling manager on window.s.

[artemlitch]

[MEDIUM] ok here now means "did not throw synchronously". instantlyScrollPageDown/Up return void, and queueScrollToPageEvent, processAllScrollEvents and _scrollToPage are all async, so any throw in the scroll path is a rejection those two methods discard. Native sees ok = YES. The rejection also leaves currentlyProcessingScrollEvents true, so no settle follows and the controller sits in AwaitingSettle with only the 3 s log. !ok is reachable only from a missing window.s, a dead web process, or a throw in suspendTtsAutoScrollForGesture / the touch_move emit.

The old nativePageCurlScrollToPage returned the queue promise. Returning it from the two methods on the JS side (return this.queueScrollToPageEvent(...)) restores the signal this branch handles.

[artemlitch]

Confirmed on the iPhone Duo simulator by making _scrollToPage reject once: native logged error=(null), the 3 s watchdog fired, and currentlyProcessingScrollEvents stayed true. The JS fix is in the rekindled worktree and not pushed yet: both methods return the queue promise, and processAllScrollEvents clears currentlyProcessingScrollEvents in a finally. With that frame the same rejection reaches !ok, and the log goes rebake requested (landing failed), awaitingSettle -> baking -> idle on the original page in 300 ms.

[artemlitch]

[LOW] The PR body states the direction-based landing as deliberate, so only the one consequence to watch: the manager computes from its own currentPage, the cover from the pre-turn _page. If the manager moved during the turn (chapter jump, resize relayout, an RN-driven page turn; the blocker stops touches only), it lands on a different page than the cover shows and the next bake swaps the cover. A page jump, no longer a wedge now that unsettled clears _awaitingLanding.

Nit: turn holds a JS method name; fn matches the rest of the file.

[artemlitch]

Renamed turn to fn in 42bbed4. No change for the page jump case: the next bake corrects the cover, as you describe.

[artemlitch]

[LOW] The landing does not go through this method any more; it calls callBridge:argument:completion: directly. This method has one caller, stepBridge:page:, which is only ever passed nativePageCurlJump (three sites). Inline it there, or make the step jump-specific, and move this comment to the general method.

[artemlitch]

Fixed in 42bbed4. The page wrapper is gone; the bake step is stepJumpToPage: and calls nativePageCurlJump itself. The comment on the general method now says only that it awaits the manager's method.

[artemlitch]

[LOW] Two things. The completion ignores failure: if this bridge call errors, the controller stays in AwaitingSettle with input blocked and nothing coming; going idle when it fails closes that. And the same call exists in requestRebake: (564); after the setState: above, [self requestRebake:@"bake failed"] reuses it, since awaitSettleIfResting: is then a no-op.

Related, JS side: invalidateNativePageCurl checks its guards, awaits two frames, then settles without re-checking. If a landing's unsettled goes out in that gap, a settle for the old page follows it with _awaitingLanding already cleared: a one-bake flip back that corrects itself. Re-check currentlyProcessingScrollEvents after nextPaint().

[artemlitch]

Fixed in 42bbed4. finishBake: calls requestRebake:, and requestRebake: now handles a failed bridge call: if the controller is still in the same AwaitingSettle, it blanks the slots and goes idle with reason "rebake request failed". Tested by making both the jump and invalidateNativePageCurl throw: baking -> awaitingSettle (bake failed), then awaitingSettle -> idle (rebake request failed) 1 ms later. The JS re-check after nextPaint() is in the rekindled worktree, not pushed yet.

[artemlitch]

[LOW] "asks the manager to settle again" holds for one of the three failure paths: the capped path goes idle and asks nothing, and the pending-settle path rebakes directly. Also "bake failed" is the reason string for two different transitions (idle at the cap, AwaitingSettle below it), so the log cannot tell them apart, and the _failedBakes ivar comment says "past the cap" where the check is >=.

[artemlitch]

Fixed in 42bbed4. The header comment lists all three failure paths, the capped path logs baking -> idle (bakes kept failing), and the ivar comment says "at the cap".