Android: opt-in removal snapshot (snapshotOnRemoval) + interactive edge-swipe-back

[artemlitch]

What

Adds an opt-in snapshotOnRemoval Screen prop (Android, default false). When enabled, the screen freezes its last presented frame — one bounded PixelCopy (≤50 ms budget, 6–15 ms measured on a Pixel 6a) — into its own ViewOverlay at the start of its removal transition, so the pop animation always shows real pixels. Released at transition end, animation cancel, or detach.

Why

The pop animation renders the departing screen while surface-backed content (e.g. a mid-fling WebView) can stop producing pixels; with a transparent background the window shows through as a black flash for the duration of the slide. In the Reader (readwiseio/rekindled#10671) this reproduced on ~50% of mid-fling back presses. Capture at WebView destroy can't fix it — teardown is JS-driven and can lag the native animation by seconds — so the capture happens where the transition actually starts.

Opt-in only: the capture reads the composited window, which is only correct for opaque full-window screens. Translucent modals/sheets would bake the underlying route into the snapshot, so the prop must never be enabled for them.

Implementation

• Screen.kt: captureScreenSnapshotForRemoval() — main-thread-marshaled, bounded-latch PixelCopy into the Screen's ViewOverlay; release in endRemovalTransition() / onDetachedFromWindow(). Timeout bitmap ownership uses an atomic handoff (whoever finishes second recycles) so a timed-out PixelCopy can never race a recycle of a bitmap it is still writing. Clipped captures are rejected rather than stretched; only the snapshot's own drawable is removed from the overlay.
• ScreenStack.kt: synchronous main-thread capture in onUpdate's close-animation branch, immediately before the removal transaction — this is the ordering guarantee. Fabric-thread callers of startRemovalTransition (observed on mqt_v_js) get a posted early capture; the first successful capture wins via the bitmap guard, and the posted path re-checks the opt-in flag.
• ScreenAnimationDelegate.kt: onAnimationCancel releases the snapshot so a cancelled exit can't leave a frozen frame over a live screen.
• Prop plumbing: Fabric spec (ScreenNativeComponent.ts), src/types.tsx, prebuilt lib/typescript/types.d.ts, ScreenViewManager setter.

Testing (Pixel 6a, physical device, via readwiseio/rekindled#10671)

• Mid-fling native back ×10 with per-frame video scan: zero black frames (previously 8–12 black frames on ~50% of cycles).
• Interrupted transitions (pop→immediate reopen ×3, rapid double-back ×3): no frozen overlay; screens fully live.
• 15-cycle pop memory soak: Java/Graphics plateau after warm-up, Native heap ends below baseline — no per-pop bitmap retention.
• Screens without the prop: no PixelCopy, no allocation, stock transitions.

🤖 Generated with Claude Code

[artemlitch]

Code Review — blocking findings

Summary

• 3 blocking issues found
• 1 HIGH, 2 MEDIUM

GitHub does not allow requesting changes on your own PR, so this is posted as COMMENT while retaining blocking severity. The architecture/codegen and Android formatting checks currently fail, and the capture lifecycle permits a late retry that violates the pre-transition snapshot invariant.

[artemlitch]

[HIGH] The new prop was not added to the checked-in Paper codegen. RNSScreenManagerInterface has no setSnapshotOnRemoval, so this override should fail Paper compilation, and RNSScreenManagerDelegate has no case that can dispatch the prop. yarn architectures-consistency-check currently fails on this exact mismatch.

Run yarn sync-architectures, commit the regenerated Paper interface and delegate, and verify the Paper Android build.

[artemlitch]

Fixed in 5879b16 — ran yarn sync-architectures and committed the regenerated RNSScreenManagerInterface/RNSScreenManagerDelegate (the diff is exactly the setSnapshotOnRemoval method + delegate case). node scripts/codegen-check-consistency.js now exits 0.

[artemlitch]

[MEDIUM] This guards only a successful capture, not whether this removal already attempted one. If the pre-transaction call from ScreenStack.onUpdate() times out or fails, the bitmap remains null; the later startRemovalTransition() entry point can retry after the fragment transition has begun. That can capture an already-translated or partly revealed window and install it mid-exit.

Track a per-removal attempted/in-flight state separately from successful bitmap ownership, allow at most one capture attempt for a removal, and reset it during transition cleanup/cancellation.

[artemlitch]

Fixed in 5879b16 — added a @Volatile removalSnapshotAttempted flag set at the first entry-point invocation (before dispatch), separate from successful-bitmap ownership, so a failed pre-transaction attempt can never be retried mid-transition by the later entry point. Reset together with the snapshot in releaseRemovalSnapshot(), which runs on transition end, animation cancel, and detach — so each new removal gets exactly one fresh attempt. Re-verified on device: mid-fling back x10, zero black frames.

[artemlitch]

[MEDIUM] The committed import order fails the repository’s Android formatter. ANDROID_HOME=/Users/artemlitch/Library/Android/sdk yarn lint-android fails spotlessKotlinCheck and requires these java.util.concurrent imports after the AndroidX/project imports.

Run Spotless (or move these imports to the formatter-required location) and rerun yarn lint-android.

[artemlitch]

Fixed in 5879b16 — moved the java.util.concurrent imports to their lexicographic position after the com.* block. ANDROID_HOME=... yarn lint-android (spotlessCheck) exits 0.

[artemlitch]

If I were implementing this

I would keep the core approach. An explicit opt-in on the screen, a synchronous pre-transaction PixelCopy, a screen-owned overlay, and idempotent lifecycle cleanup is the smallest native mechanism that directly addresses the Reader/WebView blank-frame race without changing normal stacks.

The load-bearing invariant is that the accepted snapshot must be acquired before the removal transaction starts. The current retry behavior after a failed first attempt is the one design seam I would change; a late screenshot weakens the guarantee the entire mechanism is built around.

My implementation order would be:

1. Add one-attempt-per-removal state and reset it on every completion/cancellation/detach path.
2. Regenerate and commit the Paper artifacts, then build both architectures.
3. Fix Spotless.
4. Exercise timeout, rapid repeated pop, immediate navigation/backgrounding, ordinary View Animation interruption, and repeated-removal memory behavior.

The execution within the approach is otherwise strong: it is opt-in/default-off, ownership on PixelCopy timeout is carefully handled, cleanup removes only its own drawable, and normal stack screens pay no PixelCopy or bitmap cost.

[artemlitch]

This is the right architectural seam: capture the explicitly opted-in outgoing top screen immediately before constructing the close transaction. The important invariant is that this remains the only point at which a new snapshot attempt can succeed; a later fallback attempt after failure can capture the transition itself.

[artemlitch]

The invariant is now enforced in code (5879b16): a per-removal attempted flag means no entry point can start a second capture attempt after the first — whether it succeeded or failed — until the removal's cleanup resets it.

[artemlitch]

Convention Audit — Android transition and React Native prop plumbing

Compared the new snapshot lifecycle and prop against existing Android transition, overlay-cleanup, Fabric/Paper codegen, and public-type patterns.

Matches

• Positive opt-in prop with default false across TypeScript, Fabric, and native manager layers
• Cleanup removes only the owned overlay drawable
• Normal completion and detach cleanup are idempotent
• PixelCopy timeout ownership avoids recycling while a copy may still be in flight
• Naming and lifecycle comments are consistent and clear

Deviation

• Checked-in Paper interface/delegate were not synchronized with the Fabric spec; this is enforced by the architecture consistency workflow and is covered as a blocking standard-review finding.

Novel pattern

• This is the repository’s first PixelCopy/ViewOverlay removal freeze. Its Reader-focused opt-in is an appropriate boundary.

Verification gaps

• No automated coverage for timeout ownership, duplicate entry points, cancellation/detach, or repeated removal.
• The added Animator cancellation cleanup applies to the form-sheet path; the ordinary stack View Animation interruption path still needs targeted verification.

[artemlitch]

Convention audit: no precedent — This is the repository’s first screen-removal PixelCopy/ViewOverlay mechanism. The positive opt-in/default-false prop matches existing native-prop conventions and appropriately prevents normal stack screens from paying its cost. The generated Paper surfaces still need to be synchronized, as noted in the blocking review.

[artemlitch]

Code Review

Summary

• 1 HIGH functional blocker
• 3 MEDIUM abstraction/naming findings

Blocking verdict: not ready to merge. This is posted as COMMENT only because GitHub does not permit requesting changes on your own PR.

[artemlitch]

[HIGH — BLOCKING] The off-main caller claims the sole attempt before any capture happens.

Fabric can enter this method off-main, set removalSnapshotAttempted = true, and only enqueue captureScreenSnapshotOnMain(). The synchronous safety call in ScreenStack.onUpdate() immediately before transaction creation then returns at line 508 because the queued caller already claimed the attempt. That queued runnable can consequently execute after the removal transaction starts and capture an already-translated frame—the exact state the comment above says must never be captured. A delayed runnable can also survive releaseRemovalSnapshot() resetting the flag and install an overlay in a later lifecycle.

Claim the attempt on main at actual capture time, or distinguish scheduled vs attempted and attach a removal-generation token. The synchronous pre-transaction call must be allowed to win, and an older queued runnable must no-op. Please add a deterministic test for off-main startRemovalTransition() interleaved with main-thread onUpdate().

[artemlitch]

Fixed in 16b8ea4, following the prescribed design:

• Attempt state is main-thread-owned and claimed only at actual capture time (inside captureScreenSnapshotOnMain), never at scheduling time. The synchronous pre-transaction call in ScreenStack.onUpdate() therefore always wins: it can no longer find the attempt pre-claimed by a queued off-main request.
• Off-main requests carry a removal-generation token (@Volatile removalSnapshotGeneration, incremented on main in releaseRemovalSnapshot()). A delayed runnable compares its token on main and no-ops if its removal was already cleaned up — it can never install an overlay into a later lifecycle. Within a live removal, a late-running post no-ops on the already-claimed attempt.
• All writes to arbitration state now happen on the main thread only; the off-main path reads one volatile int and posts.

On the deterministic interleaving test: the Android tree currently has no unit-test infrastructure at all (no test source sets, no junit/robolectric dependencies), so a deterministic threading test means standing up a test framework in the fork — I'd rather not couple that to this fix. The redesign removes the interleaving class structurally (single-writer main-thread state) rather than defending against it, and the device re-verification (mid-fling back ×10, zero black frames, including the Fabric off-main startRemovalTransition path interleaved with main-thread onUpdate on every cycle) passes against this exact commit. Happy to add Robolectric coverage as a separate infra PR if we want it.

[artemlitch]

[MEDIUM] snapshotHandler() is a premature single-use extraction.

It has one call site (line 558), seven lines of thin initialization/caching orchestration, and two backing fields that exist only for it. Prefer a lazy companion property (or initialize at the call site) so the handler/thread ownership is visible without a one-off method.

[artemlitch]

Done in 16b8ea4 — replaced the one-use method and its two backing fields with a lazy companion property; thread/handler ownership is now visible at the declaration.

[artemlitch]

[MEDIUM] resultHolder is too generic for concurrency-sensitive state.

Rename it to pixelCopyResult or pixelCopyResultHolder, so the value crossing the callback/waiter boundary is explicit.

[artemlitch]

Renamed to pixelCopyResult in 16b8ea4.

[artemlitch]

[MEDIUM] loserRecycles misdescribes this ownership protocol.

The flag records that one of two cleanup participants has finished; whichever arrives second recycles. Rename it around that actual invariant (for example oneCleanupParticipantFinished) and phrase the adjacent comments in the same terms.

[artemlitch]

Renamed to oneCleanupParticipantFinished in 16b8ea4, with the adjacent comments rephrased around the actual invariant: two cleanup participants exist (callback, timed-out waiter); the flag records that one has finished, and whichever arrives second owns recycling.

[artemlitch]

Convention Audit — Android rendering / lifecycle / bridge plumbing

Compared the nine-file GitHub patch at base fdd3bf39c869f96ef792097b9dab851985e20ac1 and head 5879b1629197859e097a1263dfbf2dbed542c61a against surrounding repository code.

Matches convention

• snapshotOnRemoval is plumbed consistently through the Fabric spec, Kotlin manager, generated Paper delegate/interface, and public TypeScript types.
• The default is consistently false.
• Cleanup covers normal animation completion, cancellation, and detach, and removes only the owned overlay drawable.
• The latch establishes result visibility; the atomic timeout handoff avoids recycling while PixelCopy may still write.

Novel pattern

• PixelCopy + ViewOverlay + HandlerThread snapshotting has no precedent in this Android tree. The mechanism is appropriately opt-in because it captures composited window pixels and is only correct for opaque full-window screens.

Gap

• There is no automated test for the new cross-thread/removal lifecycle state machine. Given the ordering race in the blocking review, deterministic interleaving coverage is needed.

Explicit product/performance tradeoff

• Capture can block the main thread for up to 50 ms. The PR documents physical-device measurements, but this is still a deliberate frame-time tradeoff worth retaining in the design record.

No additional convention deviations were found.

[artemlitch]

If I were implementing this

The core approach is reasonable, but I would change the capture arbitration before shipping it. PixelCopy into a screen-owned overlay is a focused opt-in solution, and the timeout bitmap-ownership handling is careful. The load-bearing weakness is not PixelCopy itself; it is representing scheduled, attempted, and belongs to this removal with one boolean shared across two threads. That makes the claimed pre-transaction ordering unenforceable and permits stale queued work to cross lifecycle boundaries.

I would implement this in this order:

1. Make the main thread the sole owner of capture-attempt state.
2. Give each removal a generation/token so delayed posts cannot affect a later removal.
3. Let the synchronous pre-transaction ScreenStack.onUpdate() path win over queued requests.
4. Add deterministic race coverage plus timeout/cancel/detach lifecycle tests.
5. Simplify the one-use handler abstraction and rename the concurrency fields so the ownership protocol reads directly.

What I would keep: the opt-in public contract, full-window restriction, screen-owned overlay, bounded wait, clipped-rectangle rejection, and explicit cleanup seams. The execution inside the approach is thoughtful; the arbitration state machine is the part that needs redesign before merge.

[artemlitch]

Architectural seam: this single boolean currently conflates a queued request, an actual capture attempt, and membership in a particular removal lifecycle. I would replace it with main-thread-owned attempt state plus a removal generation/token; that makes the pre-transaction guarantee explicit rather than timing-dependent.

[artemlitch]

Addressed in 16b8ea4 per the blocking finding: scheduled/attempted/lifecycle are now distinct — attempt state is main-thread-owned and claimed at capture time, and queued requests carry a removal-generation token. The pre-transaction guarantee is structural rather than timing-dependent.

[artemlitch]

If I were implementing this

The core placement is right, but I would change the synchronization boundary before merging. Capturing at the native removal seam is the correct ownership decision: it is earlier and more reliable than JS/WebView teardown, the prop is safely opt-in, and the cleanup is tied to end/cancel/detach. The Paper/Fabric plumbing also follows the existing generated-property shape cleanly.

The load-bearing weakness is that an asynchronous PixelCopy is made synchronous by parking the UI thread for up to 50 ms. This feature exists to protect an animation, so blocking Choreographer immediately before that animation is the wrong trade even if the measured common case is 6–15 ms. The timeout and bitmap handoff are carefully implemented within that approach; I would keep that ownership logic but move the continuation out of the blocking call.

Concrete plan:

1. Split capture request from removal continuation: request PixelCopy, then resume the transaction on the main thread from success/failure or a 50 ms timeout.
2. Keep the generation token and one-attempt invariant so stale callbacks cannot affect a later screen lifecycle.
3. Extract a small injectable capture-result boundary and test success, timeout/result races, cancel, detach, and repeated removals.
4. Keep the opt-in API and opaque/full-window warning exactly as they are.

Within the current design, the rect validation, timeout ownership handshake, idempotent cleanup, and codegen coverage are strong. I would not replace the native snapshot approach; I would make its pre-transition coordination nonblocking.

[artemlitch]

This is the architectural seam I would change: PixelCopy may remain asynchronous, but the removal transaction should be its continuation rather than making the UI thread synchronously wait here.

[artemlitch]

See the thread on line 574 — the fully-async continuation was implemented (84db105) and measured: it guarantees 1–2 black frames per mid-fling pop because the app's React-commit blank presents inside the install gap; transaction deferral can't prevent that. 9ff816c bounds the synchronous wait to one frame period with an async fallback, which preserves the nonblocking property in the worst case while keeping the flash-free property in the typical case.

[artemlitch]

Convention Audit — Android lifecycle, animation, and RN prop plumbing

Compared the exact 9-file GitHub patch against the current base branch. The removal lifecycle hooks, cancellation/detach cleanup, ReactProp setter, Fabric spec, generated Paper interface/delegate, and public TypeScript declaration match their surrounding conventions. PixelCopy/ViewOverlay has no repository precedent.

Novel Patterns Introduced

• Window-level PixelCopy into a Screen ViewOverlay
• A process-lifetime HandlerThread for capture callbacks
• Main-thread capture arbitration with a removal-generation token

The inline notes cover the actionable deviation and test gap.

[artemlitch]

Convention audit: deviation — transition lifecycle work now synchronously waits on a worker callback from the main thread.

The existing removal path is synchronous and lightweight, and nearby transition work is posted rather than waited on: ScreenFragment.kt:295 and ScreenStackViewManager.kt:38. Preserve that nonblocking property by continuing the transaction from PixelCopy completion/timeout instead of awaiting it on UI.

[artemlitch]

Convention audit: gap — this adds a new lifecycle/thread-race state machine without an automated regression case.

The timeout/callback ownership handoff, cancellation, detach, repeated removal, API <26 behavior, and Paper/Fabric ordering are the load-bearing cases. Add a focused Android test seam around the capture result/timeout and at least an app regression case that exercises removal and cancellation; the existing stack test fixture is at apps/src/tests/TestScreenStack/index.tsx.

[artemlitch]

Acknowledged as a real gap. Two constraints shaped what I did instead: the fork's Android tree has no unit-test source sets or test dependencies, and the load-bearing behavior (PixelCopy timing vs presented frames) is only observable on a real compositor. The lifecycle cases named here — timeout, cancellation, detach, repeated removal, both entry-point orderings — are covered by on-device suites that ran against each commit (interrupted-transition ×6, rapid double-back, 15-cycle removal memory soak, and a per-frame dual-probe video scan of mid-fling pops that caught a real regression in the async variant and drove 9ff816c). I'd take the TestScreenStack fixture + Robolectric seam as a follow-up PR rather than coupling test infrastructure to this change.

[artemlitch]

Code Review

Summary

• 2 issues found
• 1 HIGH (blocking), 1 MEDIUM (blocking)

This is the author's own PR, so this is posted as COMMENT; the severity labels and blocking verdict are unchanged.

[artemlitch]

[HIGH] This blocks the Android main/UI thread for up to 50 ms immediately before starting the removal animation. Both entry paths deliberately converge on captureScreenSnapshotOnMain(), so every opted-in pop can lose roughly 3 frames at 60 Hz (and more at 120 Hz) while input and Choreographer are stalled. That directly trades one transition artifact for deterministic animation jank.

Please keep the main thread nonblocking: let PixelCopy completion (or the timeout fallback) continue/defer the removal transaction, or use a capture primitive that does not require waiting on another thread.

[artemlitch]

Addressed across 84db105 → 9ff816c, with an empirical result worth recording:

I first shipped the fully-nonblocking variant (84db105): copy requested pre-transaction, main thread never waits, completion posts the install. On device it produced 1–2 fully black frames on every mid-fling pop (dual-probe per-frame video scan; the previous synchronous build showed zero such frames across 30+ cycles). The reason: the app's own teardown blank (React commit clearing the doc on beforeRemove) presents inside any async install gap. Deferring the removal transaction as a continuation would not prevent this — the blank is driven by the app's React commit, not by the transaction's timing.

So a synchronous install in the pre-transaction main-thread turn is load-bearing whenever the copy can make it. 9ff816c is the hybrid: the main thread waits at most one frame period (17 ms) — measured copies complete in 6–16 ms, so the typical install is synchronous and flash-free — and if the copy is slower it moves on, with the completion posting the install instead (single main-thread-owned install/drop decision, generation-guarded). Worst case is one frame of jank or one late frame; the 50 ms stall is gone, and the deterministic multi-frame jank concern no longer applies (bound ≤ 1 frame @60 Hz).

Re-verified on device with the stricter dual-probe detector: 10/10 mid-fling pops, zero dual-black frames.

(Final state verified against f5a1e8a, which also carries the round-4 comment-precision fixes: 10/10 mid-fling pops, dual-probe scan, zero black frames, probe values 190–201 throughout.)

[artemlitch]

[MEDIUM] removalSnapshotBitmap is dead state: it is assigned at capture, nulled at release, and never read. removalSnapshotDrawable already strongly retains the bitmap, so this second field implies an ownership/lifecycle role that does not exist.

Remove this field and its two assignments; keep the drawable as the single owner tracked by the screen.

[artemlitch]

Fixed in 84db105 — the field was removed; the drawable is the single owner tracked by the screen.

[artemlitch]

Code Review

Summary

• 1 issue found
• 1 MEDIUM (blocking)

Because this is the author’s own PR, this is posted as COMMENT while retaining blocking severity.

[artemlitch]

[MEDIUM] Regenerate the shipped Fabric declaration for this new native prop

snapshotOnRemoval is added to NativeProps here, but the exact head still ships lib/typescript/fabric/ScreenNativeComponent.d.ts without this member (and its source map is likewise unchanged). Direct consumers of the packaged Fabric native component therefore receive a stale TypeScript contract even though the runtime and public ScreenProps accept the prop. This repo normally commits the matching generated declaration/map alongside Fabric spec changes, so please run the normal build/codegen workflow and commit lib/typescript/fabric/ScreenNativeComponent.d.ts plus .d.ts.map.

[artemlitch]

Done in f5a1e8a — ran the normal bob build; the regeneration touched exactly the declarations/maps downstream of the spec change (lib/typescript/fabric/ScreenNativeComponent.d.ts + .map, lib/typescript/types.d.ts + .map, and the module/commonjs source maps), all committed.

[artemlitch]

Convention Audit — Android native, generated contracts, and tests

Compared exact base fdd3bf39c869f96ef792097b9dab851985e20ac1 with head 9ff816caf028c06c01e179f683b2faa6c4a6a644.

Matches convention

• Default-false prop plumbing is consistent across the public TS type, Fabric spec, Paper delegate/interface, and Android view manager.
• Both capture entry points converge on one implementation; all cleanup paths converge on releaseRemovalSnapshot.
• Generation-token rejection, main-thread overlay mutation, and detach/end/cancel cleanup consistently protect stale callbacks.
• The abstraction audit found no under- or over-abstraction.

Deviation / gap

• Generated-artifact gap (blocking): lib/typescript/fabric/ScreenNativeComponent.d.ts and its map were not regenerated with the Fabric source spec; the standard review anchors the actionable fix. Existing Fabric prop commits update source and generated declarations together.
• PixelCopy plus a process-lifetime callback thread and bounded latch has no repository precedent. It is a documented, load-bearing platform-specific mechanism, so this is recorded as novel rather than treated as a defect.
• No automated state-machine regression coverage was added. The repository has no established library Android unit-test harness for this path, so this is a non-blocking coverage gap rather than a convention violation.

[artemlitch]

Convention Audit — Naming and cleanliness

Three minor documentation inaccuracies remain in the new snapshot lifecycle comments. These do not change the blocking verdict, but tightening them will keep the concurrency contract accurate for future maintainers.

[artemlitch]

Convention audit: minor divergence — The timeout wording no longer matches the current async fallback.

The 17ms wait expiring is not a failed attempt: the same PixelCopy remains live and may install from its posted continuation. Please distinguish actual failure from asynchronous completion, e.g. “If the attempt fails or completes asynchronously, later entry points must not issue another capture.”

[artemlitch]

Fixed in f5a1e8a — the one-attempt comment now distinguishes outright failure from asynchronous completion, per your wording.

[artemlitch]

Convention audit: minor divergence — “Never a dropped snapshot” overstates the fallback guarantee.

installOrDropRemovalSnapshot intentionally drops/recycles on copy failure, generation change, or detach. A precise claim is that the code never abandons an in-flight successful snapshot solely because the synchronous wait elapsed.

[artemlitch]

Fixed in f5a1e8a — the claim is now precise: an in-flight successful copy is never abandoned merely because the synchronous wait elapsed; the continuation still drops on copy failure, generation change, or detach.

[artemlitch]

Convention audit: minor divergence — 17ms is not one frame period on all supported displays.

It is approximately one 60Hz frame and exceeds one frame at 90/120Hz. Calling this a “17ms bounded wait” keeps the comment accurate without implying a device-independent frame budget.

[artemlitch]

Fixed in f5a1e8a — described as a bounded wait of ~one frame at 60Hz (more than one at 90/120Hz), not a device-independent frame period.

[artemlitch]

If I were implementing this

I’d keep the core design, but I would not merge this exact package output.

The load-bearing constraint is real and explicit: the overlay has to be installed in the pre-transaction main-thread turn when PixelCopy completes within the bounded window, while a late callback must never resurrect pixels after cancellation, detach, or a later screen lifecycle. The current generation token, single-attempt arbitration, main-thread-only handled decision, and cleanup convergence express that contract clearly. The abstraction audit also found no duplicate path or premature wrapper: the small helpers correspond to actual concurrency/lifecycle boundaries.

The 17ms bounded wait is a deliberate platform tradeoff rather than an accidental blocking call. The current commit documents the observed failure of the fully asynchronous variant, and there is no concrete repository-local alternative that preserves the “no teardown gap” invariant, so I would keep it pending device acceptance rather than turn that into a speculative design objection.

What I would change before merge is the release boundary: regenerate the Fabric declaration/map so the shipped package matches the source spec. Then tighten the three comments called out by the naming audit so they describe asynchronous completion, bitmap drops, and the fixed 17ms budget precisely. Finally, I would record a focused manual acceptance matrix for 60/90/120Hz devices covering successful synchronous capture, late success, copy failure, cancellation, and detach. That protects the actual invariant without forcing a new Android test harness into this PR.

Concrete order:

1. Regenerate and commit lib/typescript/fabric/ScreenNativeComponent.d.ts and its map.
2. Correct the timeout/drop/frame-period comments.
3. Run the mid-fling removal matrix across refresh rates and retain the current lifecycle design if it holds.

[artemlitch]

Code Review

Summary

• Reviewed the exact GitHub patch at head f5a1e8a7cdfa8b9c9339c84740b0d2b2e4c3ef2a against base fdd3bf39c869f96ef792097b9dab851985e20ac1.
• No high-confidence correctness, lifecycle, concurrency, or generated-artifact defect found.
• Verified the pre-transaction/main-thread capture arbitration, generation-guarded async completion, end/cancel/detach cleanup, API/clipping/allocation/failure guards, Paper/Fabric prop plumbing, and shipped declarations/maps.
• The opt-in path intentionally spends up to 17 ms on the UI thread to prioritize a pre-animation snapshot; that is a documented product/performance tradeoff, not an unbounded stall.

Checks: yarn architectures-consistency-check passed; yarn check-types passed; git diff --check passed. Android Spotless could not configure in the isolated checkout because the Android SDK path/compile SDK environment is absent. GitHub reports no checks.

[artemlitch]

Convention Audit — Android / generated contracts / cleanliness

The Android lifecycle shape, boolean prop plumbing, Paper/Fabric bridge, cleanup entry points, and regenerated shipped declarations/maps match the surrounding project conventions. The PixelCopy + bounded main-thread handoff is a novel pattern in this tree, but its ownership and stale-callback guards are internally coherent.

Two concrete documentation cleanups remain:

• The PR description still says ‘≤50 ms’ and describes a removed two-participant atomic/recycle handoff. The current head waits 17 ms and serializes install/drop ownership on the main thread.
• The public prop wording should describe capture as best-effort because PixelCopy can complete asynchronously or fail.

No abstraction-fit, naming, dead-code, export, or generated-artifact drift was found.

[artemlitch]

Convention audit: minor divergence — This public contract currently reads as an unconditional, start-of-transition freeze, but the implementation explicitly allows the 17 ms wait to elapse and installs from an async continuation; PixelCopy failure also drops the bitmap. Please describe this as a best-effort capture requested at removal start (and mirror the wording in the generated declaration via the normal build), so consumers do not rely on a guarantee the native path cannot make.

[artemlitch]

Fixed in d085779 — the prop doc (src/types.tsx and the bob-regenerated shipped declarations) now describes a best-effort capture requested at removal start: asynchronous completion shortly after the transition begins is possible, and the capture is dropped on PixelCopy failure or when the view has left the window.

[artemlitch]

If I were implementing this

I’d keep this design. The load-bearing choice is capturing at ScreenStack.onUpdate() immediately before the close transaction, while retaining startRemovalTransition() as a fallback lifecycle entry point. removalSnapshotAttempted makes those two entry points converge on one attempt, and the generation token plus end/cancel/detach cleanup gives the asynchronous PixelCopy completion a clear ownership boundary.

I would not replace this with WebView teardown capture: teardown is not ordered tightly enough with the native fragment transaction, while this code is. I also would not add another abstraction around the capture state machine; the helpers correspond to real thread/lifecycle boundaries and have multiple call sites.

Concrete plan:

1. Keep the current capture and cleanup architecture.
2. Correct the PR body and public prop wording to reflect the actual 17 ms, best-effort async-fallback behavior.
3. Add focused Android tests around delayed completion after release/detach and multi-remove behavior when a practical PixelCopy seam is available.
4. Preserve the opt-in/default-off contract and the opaque full-window restriction.

The implementation within this approach is careful: generated Paper/Fabric and packaged type artifacts are synchronized, clipped captures are rejected, stale completions cannot install into a later lifecycle, and all terminal animation paths remove the overlay.

[artemlitch]

Added: interactive edge-swipe-back (gesture-only port of PR #3)

cc9ab08f ports PR #3's EdgeSwipeBackController with every programmatic-back concern removed. The native gesture diff is exactly two files: EdgeSwipeBackController.kt (new) and ScreenStack.kt (touch delegation, pair resolution, onStackChildrenChanged() hook). ScreenStackFragment.kt is untouched — no OnBackPressedCallback, no dismissTopWithAnimation() — so hardware/system/header back stay on the snapshot-protected pop path this PR already ships.

Design points:

• Arming reuses Screen.isGestureEnabled exactly as PR Android: native interactive edge-swipe-back + native-first back-button dismissal #3 did (dead field on Android otherwise). Native-stack passes false on Android unless a screen sets the new patch-level androidEdgeSwipeBack option and isn't remove-prevented — only the Reader opts in. No new native prop, no Fabric/Paper codegen churn.
• ACTION_DOWN is tightened to cheapest-first rejection while preserving state-machine priority: SETTLING/AWAITING_REMOVAL guards first, then the 50dp band check, then isGestureEnabled, push check, and pair resolution dead last — non-opted-in screens (all of Bookwise) pay one float compare per touch.
• No swipe/snapshot coordinator: a committed swipe leaves the screen fully off-window, so the existing srcRect.intersect(...) empty-intersection check rejects the removal capture before any allocation.
• Committed-settle → notifyTopDetached() → ScreenDismissedEvent → source-keyed StackActions.pop, which is a router no-op if a concurrent back press already removed the route (verified against StackRouter's findIndex on action.source).

Device verification (Pixel 6a, physical):

• Interactive tracking confirmed as controller-driven (mid-drag screenshots show 1:1 finger tracking with the live below-screen parallaxed at −30%→0 — not a post-release animation), under both 3-button and gesture navigation; on gesture nav the outer bezel correctly performs a plain system back (zero black frames).
• Commit rule (dx + 0.3·v > w/2), cancel-restore-to-identity, vertical-intent bail, out-of-band touches fall through to the app's own gestures.
• Root screen refuses (pair resolution) and the touch passes through cleanly.
• Over-pop: double-swipe and back-mash during committed settle both pop exactly one route; zero awaiting-removal safeguard fired warnings across all runs (JS always reconciled <1s).
• Snapshot regression gate: mid-fling hardware back ×10 → 10/10 clean (probe YMAX 200–208, zero black runs); committed-swipe recording has zero black frames and no second exit animation.
• Bookwise smoke (shares the pin, no route opts in): push/pop, reading view, vertical scroll, horizontal shelf gestures all normal; left-edge drags completely inert (mid-drag frame pixel-identical to rest state).

🤖 Generated with Claude Code

[artemlitch]

Code Review

Summary

• 1 issue found
• 1 HIGH / BLOCKING

GitHub does not allow requesting changes on your own PR, so this is posted as COMMENT while preserving blocking severity. The edge-swipe timeout recovery must be fixed before merge.

[artemlitch]

[HIGH — BLOCKING] This safeguard releases the state machine without restoring the native stack.

On the committed path, finish(true) has moved the top screen to width, attached the below fragment, cleared topScreenView/belowScreenView, and entered AWAITING_REMOVAL. If JS never reconciles—the exact case this timeout handles—changing only state to IDLE leaves topScreen pointing at an offscreen view while the below fragment remains attached. The next touch/gesture therefore runs against a structurally inconsistent stack.

Retain the committed pair until reconciliation. On timeout, either roll back fully (restore the top transform, detach the exact below fragment, clear refs/tracker, then return to IDLE) or keep interaction blocked and surface the failed reconciliation. Please add coverage for this timeout path so the recovery invariant is executable.

[artemlitch]

Fixed in d085779, and verified the way this finding deserved. The committed (top, below) pair is now retained through AWAITING_REMOVAL (finish(true) no longer clears the refs); on timeout the rollback restores the top screen's transform, detaches the below fragment, clears the refs, and returns to IDLE. Reconciliation (onStackChildrenChanged) drops the retained pair without touching transforms, preserving the no-flash asymmetry.

Verification: I froze the app's JS thread via the Hermes debugger (CDP Debugger.pause through Metro) and performed a committed edge swipe — exactly the "JS never reconciles" case. Logcat shows the rollback firing at 1s, and the screen shows the Reader restored at identity while JS is still frozen; on resume, the queued ScreenDismissedEvent pops normally. Notably, the first cut of this fix (rollback without retaining the pair) produced a black screen in this very test — the refs were already nulled so the restore no-oped — which is empirical confirmation that pair retention is the load-bearing part of your prescription.

On the coverage ask: the fork's Android tree still has no test source sets/dependencies, so this remains harness-limited to the live-device procedure above (scripted and repeatable: freeze_js.js via Metro's inspector endpoint).

[artemlitch]

Convention Audit — Android integration and cleanliness

The native state-machine extraction and generated prop plumbing fit the repository. Two non-blocking coupling/cleanliness notes remain around the new edge-swipe contract.

[artemlitch]

Convention audit: coupling — This controller's arming contract depends on an androidEdgeSwipeBack/usePreventRemove mapping that is not present in this repository.

The current native-stack source still forces gestureEnabled={false} on Android, while the PR description explains that Reader supplies a separate patch-level mapping. That is intentional fork integration, but it makes this native code unsafe to consume independently: the prevent-remove guarantee exists only in the downstream patch. Please make that dependency durable—either add the typed mapping here or link/name the downstream patch contract explicitly in code and keep the two changes pinned together.

[artemlitch]

Fixed in d085779 — the controller header now names the downstream contract durably: it states that upstream native-stack forces gestureEnabled=false on Android (so the controller ships disarmed for independent consumers), and points at readwiseio/rekindled's patches/@react-navigation__native-stack@7.12.0.patch with the exact arming expression, explicitly noting the prevent-remove guarantee lives in that patch. The two sides stay pinned together by the app's commit-pinned dependency on this fork.

[artemlitch]

Convention audit: minor divergence — master's reader gesture band is app/branch-specific terminology inside a library controller.

Describe this as the controller's 50dp Android edge region (or link a durable shared contract). Otherwise a later Reader hit-slop change can make this comment silently false even though the constant remains unchanged.

[artemlitch]

Fixed in d085779 — the constant is now described as the controller's own Android edge region (system-back bezel note retained), with no app/branch-specific terminology.

[artemlitch]

If I were implementing this

The core approach is right, but I would tighten the ownership boundary before merging. A dedicated EdgeSwipeBackController is the correct shape: gesture arbitration, fragment exposure, transforms, settling, and JS-dismiss handoff form one coherent native state machine, and keeping that machinery out of ScreenStack is materially clearer than spreading callbacks across the container. The snapshot path is also deliberately opt-in and its source/generated plumbing is synchronized.

The load-bearing weakness is recovery ownership. A committed gesture transfers visual ownership before JS confirms the pop, but the controller discards the screen pair and later treats a timeout as if releasing an enum were enough. That is why the timeout defect is structural rather than a small cleanup bug. The other seam is cross-repo enablement: this repo's native-stack still disables Android gestureEnabled; the actual opt-in and usePreventRemove gating live in a downstream Reader patch, so correctness currently depends on two repositories moving together.

My concrete plan:

1. Keep the committed pair until either the expected stack mutation arrives or timeout rollback completes.
2. Make timeout rollback restore transforms and fragment attachment atomically; add a focused regression test for no-reconciliation and mutation-during-settle paths.
3. Encode the Android opt-in/prevent-remove contract in one typed layer, preferably here; if the fork intentionally keeps it downstream, add a durable explicit link and pin.
4. Keep the controller extraction. I would not inline this into ScreenStack; after correctness is locked down, optionally give the roughly 140-line PixelCopy lifecycle the same containment via a RemovalSnapshotController, but I would not block this PR on that refactor.

Within that shape, the pair validation, animator-cancel guard, generation-based snapshot cleanup, and Fabric/Paper artifact synchronization are strong execution choices.

[artemlitch]

This is the architectural handoff to protect: after dispatching dismissal, retain enough native state to either confirm reconciliation or reverse this commit completely. Clearing the pair before that outcome is known makes the timeout incapable of restoring the invariant.

[artemlitch]

Code Review

Summary

• 4 blocking issues found on exact head d085779
• 3 HIGH correctness/protocol defects
• 1 MEDIUM rule-of-three duplication

This is an own-PR COMMENT fallback; the HIGH and MEDIUM labels remain blocking.

[artemlitch]

[HIGH] Do not treat every container update as proof that the committed pop reconciled.

ScreenStack.onUpdate calls this method unconditionally, including updates caused by activity-state or other child changes. In AWAITING_REMOVAL this branch clears the retained pair, cancels the rollback timeout, and returns to IDLE without checking that the committed top was actually removed. An unrelated update can therefore leave the logical top translated fully offscreen, allow another swipe to over-pop, and permanently disable the safeguard.

Keep AWAITING_REMOVAL until the retained top is no longer the logical/contained top. Pass the current stack identity into this transition or query it here; only then cancel the timeout and clear the pair.

[artemlitch]

[HIGH] Abort ARMED when the stack changes too.

A stack update between ACTION_DOWN and activation currently leaves the old tracker/down coordinates armed. The next MOVE calls startDrag, which resolves the new pair but does not re-check the new top gestureEnabled or presentation, so a stream armed for the previous route can steal and dismiss a newly installed or disabled route. Include State.ARMED in this invalidation path and call disarm().

[artemlitch]

[HIGH] Publish transition progress and cancellation for this manual interactive transition.

This path only mutates translationX, so it bypasses the existing fragment animator path that emits ScreenTransitionProgressEvent. As a result, useTransitionProgress/Reanimated consumers stay frozen during the drag, and a cancelled swipe emits no gestureCancel signal even though NativeStackView exposes that lifecycle. The existing Android interactive-transition implementation in ScreensModule.updateTransition dispatches progress on every update.

Dispatch normalized progress through the existing screen/fragment event path while tracking, and emit the appropriate terminal/cancel lifecycle signal when settling completes.

[artemlitch]

[MEDIUM] Consolidate the repeated reset-to-idle cleanup.

The same translation reset, retained-view clearing, and IDLE transition now appears in the timeout rollback, cancel finish, and stack-change abort paths. This has reached the rule-of-three threshold, and cleanup drift is especially risky in a state machine.

Extract a resetViewsAndIdle helper, optionally recycling the tracker, while keeping path-specific detach/logging at each caller.

[artemlitch]

Convention Audit — Android gesture, snapshot, and generated contracts

Compared the exact head against surrounding Android interactive-transition, fragment lifecycle, prop/codegen, and cleanup conventions.

Matches

• snapshotOnRemoval follows the existing Fabric -> view-manager -> Paper interface/delegate prop chain, including shipped declarations/maps.
• Pair validation correctly protects the positional attachBelowTop/detachBelowTop contract.
• Animator cancellation guards against cancel followed by end.

Deviations / gaps

• The new manual edge-swipe path bypasses the existing transition-progress event protocol (posted as a blocking standard-review finding).
• No automated Android tests cover the five-state gesture controller, unrelated updates while awaiting removal, timeout rollback, or late PixelCopy completion/generation cleanup. These are the highest-leverage regression cases for the state defects above.

Novel patterns

• PixelCopy-backed removal overlay is new in this repository; generation gating and detach cleanup are internally coherent.
• Downstream native-stack arming is intentionally external and is not raised as a repository gap.

[artemlitch]

Convention audit: minor divergence — Name this rollback for what it does.

releaseAwaitingRemoval resets transforms and detaches the below-top fragment; it performs a full rollback, not a release. Rename it to rollbackAwaitingRemoval and update the force-release comment so future lifecycle edits do not mistake this for state-only cleanup.

[artemlitch]

Convention audit: redundant — Remove the semantic alias that is permanently equal to touchSlopPx.

activationPx suggests an independently tuned activation threshold, but it is only touchSlopPx under a second name. Compare against touchSlopPx directly, or introduce a genuinely independent activationThresholdPx if separate tuning is intended.

[artemlitch]

Convention audit: minor divergence — Make the one-attempt comment match when the attempt is consumed.

The code sets removalSnapshotAttempted before attachment, size, activity, and window prerequisites, while this comment says it is claimed only at actual capture time. Document that entering the main-thread routine consumes the attempt and a prerequisite failure is final, or move the assignment after prerequisites if retry is intended.

[artemlitch]

If I were implementing this

The core split is right, but I would change the edge-swipe state machine before merging. Screen-level PixelCopy is the correct ownership boundary for snapshotOnRemoval: it is opt-in, generation-gated, cleaned up at transition end/cancel/detach, and its Fabric/Paper contract is complete. EdgeSwipeBackController is also the right place for Android touch arbitration.

The load-bearing weakness is that the controller currently acts as a parallel transition system rather than participating fully in the repository transition protocol. It moves pixels directly but does not publish transition progress/cancellation, and two lifecycle transitions are keyed to any update rather than the identity of the route that armed or committed the gesture. That is why unrelated updates can either preserve a stale ARMED stream or prematurely release AWAITING_REMOVAL. The repeated cleanup blocks are a symptom of the same issue: state ownership is spread across branches.

Concrete plan, in order:

1. Make gesture states identity-bearing: retain the armed/committed top wrapper or screen and validate that exact identity on activation and reconciliation. Abort ARMED on any stack change; release AWAITING_REMOVAL only when that retained top is actually gone/no longer logical top.
2. Route drag progress and cancel/terminal signals through the existing screen transition event path used by ScreensModule/ScreenFragment, so useTransitionProgress and navigation gesture lifecycle stay coherent with the pixels.
3. Centralize transform/ref/tracker cleanup into one reset transition, leaving detach versus committed-pop behavior at the callers.
4. Add deterministic state-machine tests for stack mutation while ARMED, unrelated update while AWAITING_REMOVAL, timeout rollback, cancel followed by animator end, and snapshot late-completion generation cleanup.

I would preserve the snapshot implementation and the dedicated controller, then tighten those lifecycle seams rather than redesigning the feature or pulling the intentionally downstream native-stack arming contract into this repository.

[artemlitch]

Implementation opinion anchor — Keep the dedicated controller as the touch/state ownership boundary, but make it participate in the existing transition event protocol and bind ARMED/AWAITING_REMOVAL to an exact screen identity.

[artemlitch]

Implementation opinion anchor — This screen-level opt-in is the right boundary for the removal snapshot. I would preserve this design while fixing the edge-swipe lifecycle seams described in the review body.