fix(android): NaN-box UI handles, safe preferencesGet, status-bar host pad#6317
Conversation
…k symbols Button presses crashed after UI open (RenderThread destroyed-mutex / SIGSEGV) because invoke* cast NaN-box bits to pointers. Match iOS/macOS: use js_nanbox_get_pointer in callback, pointer, drag_drop, state onChange, and app activate/terminate paths. Leave render_for_each raw bitcast (documented non-nanbox). Also export perry_ui_text_create_with_id / perry_ui_set_text on Android (parity with other platforms for Text(id)/setText), and link -landroid so ANativeWindow_fromSurface resolves at dlopen. Verified on Cuttlefish x86_64 (Start/Pause/Reset stopwatch) and phone arm64.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAndroid callback paths now unbox NaN-boxed closure pointers through a native helper. Widget FFI handles use JavaScript-compatible floating-point decoding, preference reads handle multiple Java types, and Android text, styling, window, and linker behavior are updated. ChangesAndroid runtime and UI integration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/perry-ui-android/src/callback.rs (1)
139-139: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding the tag-bit warning comment to the other invoke variants.
invoke0has an excellent three-line comment explaining whyto_bits()leaves the0x7ffdtag and crashes. The other variants (invoke1,invoke2,invoke4,invoke_with_string_array) perform the same unboxing without any comment, making it less obvious to future maintainers whyjs_nanbox_get_pointeris required here.A brief one-line comment (e.g.
// Unbox NaN-boxed closure — see invoke0 for details.) in each variant would prevent someone from "simplifying" back toto_bits().Also applies to: 153-153, 168-168, 185-185
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-ui-android/src/callback.rs` at line 139, Add a brief explanatory comment before the js_nanbox_get_pointer call in invoke1, invoke2, invoke4, and invoke_with_string_array, matching invoke0’s guidance that NaN-boxed closures must be unboxed through this helper rather than to_bits(). Keep the existing unboxing logic unchanged.crates/perry-ui-android/src/app.rs (1)
362-362: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueNaN-box unboxing in lifecycle handlers looks correct.
The switch from
to_bits()tojs_nanbox_get_pointer()inhandle_activateandhandle_terminateis consistent with the pattern used incallback.rs,pointer.rs,drag_drop.rs, andstate.rs. The FFI signaturefn js_nanbox_get_pointer(value: f64) -> i64matches the declaration in the other files.One minor inconsistency: unlike
pointer.rsanddrag_drop.rs, these handlers don't null-checkptrafter unboxing before callingjs_closure_call1. Since the callback was stored from a validf64, a null return would indicate a nanbox helper bug rather than a missing callback — but adding a guard would make the defensive posture uniform across all callback paths.Also applies to: 410-411, 423-423
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-ui-android/src/app.rs` at line 362, In the lifecycle handlers handle_activate and handle_terminate, add the same null-pointer guard used by pointer.rs and drag_drop.rs after js_nanbox_get_pointer() unboxes the callback value, before invoking js_closure_call1; preserve the existing callback behavior for valid pointers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/perry-ui-android/src/app.rs`:
- Line 362: In the lifecycle handlers handle_activate and handle_terminate, add
the same null-pointer guard used by pointer.rs and drag_drop.rs after
js_nanbox_get_pointer() unboxes the callback value, before invoking
js_closure_call1; preserve the existing callback behavior for valid pointers.
In `@crates/perry-ui-android/src/callback.rs`:
- Line 139: Add a brief explanatory comment before the js_nanbox_get_pointer
call in invoke1, invoke2, invoke4, and invoke_with_string_array, matching
invoke0’s guidance that NaN-boxed closures must be unboxed through this helper
rather than to_bits(). Keep the existing unboxing logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c59920ec-6f59-4e12-8742-6f5b530c447d
📒 Files selected for processing (7)
crates/perry-ui-android/src/app.rscrates/perry-ui-android/src/callback.rscrates/perry-ui-android/src/drag_drop.rscrates/perry-ui-android/src/ffi/basic_widgets.rscrates/perry-ui-android/src/pointer.rscrates/perry-ui-android/src/state.rscrates/perry/src/commands/compile/link/build_and_run.rs
Add unbox comments on remaining invoke* helpers and null-check lifecycle callback pointers after js_nanbox_get_pointer.
declare-function styling calls pass POINTER-tagged f64 handles (0x7FFD…payload). i64 parameters left the real handle in xmm while C read rdi garbage, so set_background_color never found widgets. - decode_js_handle_f64 for POINTER + INT32 tags - style FFIs take handle: f64 - MaterialButton backgroundTintList so fills actually show
preferencesGet used getString first on SharedPreferences. When a key was stored as float (preferencesSet with a number), Android threw ClassCastException and left a pending JNI exception, which poisoned the rest of nativeMain (text/hstack create panics, half the UI missing). Read via getAll and branch on String/Float/Double/Integer/Long, clearing any leftover exception before return. Also stop the activity template from using FLAG_LAYOUT_NO_LIMITS without real safe-area insets: pad the host FrameLayout by status_bar_height so content is not under system icons on Android 15 / OnePlus.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@crates/perry-ui-android/template/app/src/main/java/com/perry/app/PerryActivity.kt`:
- Around line 41-62: Make the status bar padding in PerryActivity conditional on
the window layout mode: when setDecorFitsSystemWindows(true) is used on API
30–34, do not apply statusPad because system insets already position the content
below the bar; only retain the padding for the edge-to-edge path, or remove it
if decor-fits behavior is intended. Update the rootLayout.setPadding call and
the surrounding decor configuration consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 31c1f38a-f7db-4610-8271-c141dcf8b935
📒 Files selected for processing (6)
crates/perry-ui-android/src/ffi/image_nav.rscrates/perry-ui-android/src/ffi/tabbar_layout.rscrates/perry-ui-android/src/ffi/text_scroll.rscrates/perry-ui-android/src/system.rscrates/perry-ui-android/src/widgets/mod.rscrates/perry-ui-android/template/app/src/main/java/com/perry/app/PerryActivity.kt
tangledcircuitweb
changed the title
|
thanks @tangledcircuitweb I'm quickly fixing lint + the minor finding by coderabbit and then I'll merge :) Great addition! |
Two follow-ups on the review: - `cargo fmt` drift in `ffi/basic_widgets.rs` (the two new text-registry call sites) — this is what the `lint` gate was failing on. - The host `FrameLayout` padded unconditionally by `status_bar_height`. That is only correct in the edge-to-edge regime. The template targets SDK 35, so on Android 15+ `setDecorFitsSystemWindows(true)` is a no-op and nothing insets the content — the pad is needed. But on API 30-34 that call is honored (and below 30 the decor fits by default), so the content view is already inset and the extra pad is just blank space at the top. Pad from the insets actually dispatched to the host instead of branching on SDK_INT: the decor consumes them when it fits system windows (so we pad by zero) and passes them through when it does not. Also picks up the nav bar, display cutouts and landscape, which a top-only status_bar_height pad missed. Verified by building the rendered template with AGP 8.8.2 / SDK 35.
2 participants
Summary
Hardening the Android UI backend so TS-side NaN-boxed handles and SharedPreferences number keys no longer crash or silently no-op. Proven on Cuttlefish (x86_64) and OnePlus (arm64) with a real stopwatch app.
These are Android-only (
perry-ui-android+ activity template). They align invoke/style paths with iOS/macOS unbox behavior and fix a classic typed-prefs JNI footgun.Changes
js_nanbox_get_pointerbeforejs_closure_call*on button/pointer/drag_drop/state/app lifecycle (match iOS/macOS). Rawto_bits() as *const u8left the0x7ffdtag in the pointer and corrupted the heap — UI could open, any press died on RenderThread (FORTIFY: pthread_mutex_lock called on a destroyed mutex/ SIGSEGV).f64handle args) soset_background_color/ corner radius / etc. find widgets; MaterialButton usesbackgroundTintListso solid fills actually paint.perry_ui_text_create_with_id/perry_ui_set_texton Android forText(id)+setText.-landroidsoANativeWindow_fromSurfaceresolves at dlopen.getStringon float-stored keys (ClassCastException left a pending JNI exception → subsequent text/hstack creates panicked and half the UI vanished). Read viagetAlland branch on String/Float/Double/Integer/Long; clear leftover exceptions.FLAG_LAYOUT_NO_LIMITSwithout real safe-area insets;setDecorFitsSystemWindows+ pad hostFrameLayoutbystatus_bar_heightso titles are not under system icons on Android 15 / OnePlus. (getSafeAreaInsetsstill returns zeros on Android.)Out of scope / not proposed as platform defaults
hardwareAccelerated=falsefor AdrenoCircularRRectOpaborts on rounded rects (device workaround; kept in sample app only).setInterval(separate runtime/emulator story).Related issue
n/a — found while dogfooding Android phone + Cuttlefish. Happy to file issues if you prefer them linked after the fact.
Test plan
perry-ui-android(+ runtime/stdlib as needed) forx86_64-linux-androidandaarch64-linux-androidwith global-dynamic TLSadbwireless, x86_64 APK): app opens; Start/Pause/Reset; logcat shows cleaninvoke0without high0x7ffdptr tag; no RenderThread crashpreferencesSet+ relaunch: full UI still builds (no pending ClassCastException)cargo build --releaseclean (contributor machine; CI will re-check)Screenshots / output
Phone (arm64): DaisyUI-styled stopwatch with live clock, status-bar clearance, Start → Running with advancing elapsed. Prefs restore of elapsed no longer blanks half the tree.
Checklist
fix(android):conventional-style prefixesSummary by CodeRabbit
New Features
Bug Fixes