Conversation
On Samsung One UI, split-window panes are assigned a density different from fullscreen (e.g. 321 vs 420 dpi). Two measurement pipelines in the app convert with the process-global main-display density captured at process start, while Fabric mounts with the activity's window density: 1. react-native-screens FabricEnabledViewGroup.updateState converted the measured pane px back to dp with the process-global value, collapsing all ScreenStack content into a top-left box of pane*density-ratio (upstream software-mansion/react-native-screens#4159, fixed in 4.26.0). Pin react-native-screens to 4.26.2 - the first fix line that remains compatible with react-native 0.83 codegen (4.27+ needs newer codegen), plus a pnpm patch respelling one React.ComponentRef command argument that the 0.83.10 codegen parser rejects. 2. RN's process-global DisplayMetricsHolder stays frozen at process start. Text measurement (PixelUtil.pxToDp) and all other PixelUtil conversions then disagree with the mount density by exactly the density mismatch, so text is laid out into boxes that are the wrong fraction of the glyphs - clipped lines, cut descenders. Add an android-multiwindow-metrics config plugin that re-seeds BOTH holders from the windowed resources on create/resume/configurationChange, emits didUpdateDimensions, requests a decorView relayout, and adds density to configChanges so the switch reaches the hook without activity recreation. Unlike the DeX variant in stablyai#16018, the screen holder also takes windowed metrics, which the text pipeline requires. Verified on Samsung foldable hardware (One UI, Android 16): layout and text fill the pane correctly in split panes and pop-up windows, across recreation-free fullscreen<->split transitions in both directions. Co-Authored-By: CCCha <[email protected]> via Claude Code
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe mobile app registers a new Android multi-window metrics plugin. The plugin updates activity configuration and injects Kotlin display-metric handling into Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The Android multi-window metrics update is ready to merge with normal checks. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description explains the problem, root cause, fix, and hardware verification in detail, but it does not follow the required template. It omits the required Linked Issue, Visual Proof, Testing, Review, Notes, checklist, and Agent skill upstream boundary sections. Resolution Restructure the description using the repository template. Add the required section headings and content, including a linked issue after “Fixes #”, visual proof or Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 1 files. (4 skipped: 4 unsupported.)
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 |
![pullfrog[bot]](/web-api/iframe-proxy?url=https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
mobile/app.json fails oxfmt --check, so the Mobile Checks format:check step (oxfmt --check .) will fail on this PR. Revert the formatting-only edits (or run pnpm format in mobile/).
Reviewed changes
react-native-screenspin + patch — pins the dependency to exactly4.26.2and patchesStackHeaderConfigAndroidNativeComponent.tsso RN 0.83.10 codegen accepts the command'sviewReftype.- Multi-window metrics plugin — new
mobile/plugins/android-multiwindow-metrics.jsreseeds bothDisplayMetricsHolderholders from the activity's windowedresources.displayMetricson create/resume/configuration change, requests a decor relayout, emitsdidUpdateDimensions, and addsdensityto the activityconfigChanges. - Registration + lockfile — plugin registered in
app.json;patchedDependenciesand the lockfilepatch_hashupdated to match.
ℹ️ Exported plugin transforms have no tests
injectDisplayMetrics and applyMultiWindowMetricsManifest are exported specifically for testing, but no test accompanies them. They do fragile string surgery (import insertion, super.onCreate(null) insertion, lastIndexOf('\n}') class-end detection) against an Expo template this repo does not pin, and no CI path runs expo prebuild — so template drift is only caught at Android release-build time (the plugin throws, which is at least loud). The sibling android-respect-rotation-lock.js is a single manifest write; this one is materially more logic.
Technical details
# Add unit tests for the multi-window metrics plugin
## Affected sites
- `mobile/plugins/android-multiwindow-metrics.js:55-84` — `injectDisplayMetrics` string surgery
- `mobile/plugins/android-multiwindow-metrics.js:86-95` — `applyMultiWindowMetricsManifest`
- `mobile/vitest.config.ts` — `include` only collects `src/**/*.test.ts(x)`, so a test must live under `src/` (or the include extended)
## Required outcome
- Cover the pure transforms with a small fixture resembling the Expo SDK 55 `MainActivity.kt`: assert idempotency (the `ORCA_MULTIWINDOW_METRICS` marker short-circuits), that both imports and both methods land inside the class, that `applyActivityDisplayMetrics()` precedes `super.onCreate(null)`, and that `density` is appended to (and not duplicated in) `configChanges`.
- Assert the throw paths for a missing `super.onCreate(null)` and a non-Kotlin MainActivity.
## Open questions for the human
- Is there a reason plugin tests are intentionally omitted (e.g. no prebuild harness), or was this just not done?ℹ️ Nitpicks
- The reason for the exact
[email protected]pin and its patch exists only in the PR description.pnpm-workspace.yaml'spatchedDependencieskey and the codegen constraint mean the next bump fails loudly, but a one-line note next to the pin (or a tracking issue for RN/react-native-screensfixing the codegen parser) would save the next maintainer the investigation. Upstream[email protected]still emitsReact.ComponentRef, so this is not a "bump fixes it" situation.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
| { | ||
| "NSPrivacyAccessedAPIType": "NSPrivacyAccessedAPICategoryUserDefaults", | ||
| "NSPrivacyAccessedAPITypeReasons": ["CA92.1"] | ||
| "NSPrivacyAccessedAPITypeReasons": [ |
There was a problem hiding this comment.
This reformatting is unrelated to the fix and fails the mobile formatter: pnpm format:check (oxfmt --check) wants the one-line ["CA92.1"] form back. The same applies to the C617.1 / 35F9.1 arrays and to android.permissions in this file (I reproduced the failure by running oxfmt --check app.json from mobile/). Keep the plugin entry at line 94 and revert these formatting-only edits, or run pnpm format in mobile/.
1 participant
Problem
On Samsung One UI foldables, every multi-window mode — split panes on both displays, pop-up windows — rendered the app squeezed into the top-left ~76% of its pane, with the remainder painted by the app's own background color. Window content (FAB position, list widths) tracked the shrunken box. Full-screen was fine. Stack: expo 55, react-native 0.83 (new arch), react-native-screens 4.24.
Root cause
One UI assigns split-window panes a density different from fullscreen (e.g. 420 → 321 dpi). Two conversion pipelines read the process-global main-display density captured at process start, while Fabric mounts with the activity's window density. Any multi-window transition that skips activity recreation leaves the two sides disagreeing by exactly the density ratio (321/420 ≈ 0.764 — the observed ~76% box):
FabricEnabledViewGroup.updateStateconverted the measured pane px back to dp withPixelUtil(process-global), collapsing all ScreenStack content into the top-left box. Upstream: [Android][Fabric] Screen content collapses to 1/density size on displays with a different density (Samsung DeX / freeform) — px→dp in updateState uses global density software-mansion/react-native-screens#4159, fixed in 4.26.0 by Fix oversized daemon stream output drops #4160/fix: use base ref details for review creation #4169.TextLayoutManager/ReactTextView) converts px→dp throughPixelUtilas well, againstDisplayMetricsHolder's frozen values. Even with correct Screen frames, glyphs were laid out into boxes off by the same 0.764 factor — clipped lines, cut descenders. RN core never re-seeds the holders without activity recreation (🖥️ [Android] PixelUtil uses getScreenDisplayMetrics() instead of getWindowDisplayMetrics() — breaks desktop/freeform/secondary display mode in RN 0.83+ react/react-native#55659 class).Fix
react-native-screensto 4.26.2 — the first fix line compatible with RN 0.83 codegen. Caret ranges resolve to 4.27+, whose codegen spec the 0.83 parser rejects; 4.26.2 additionally needs a pnpm patch respelling oneReact.ComponentRefcommand argument to the spelling 0.83.10 codegen accepts (included).mobile/plugins/android-multiwindow-metrics.js:DisplayMetricsHolderholders from the activity's (windowed)resources.displayMetrics, emits a freshdidUpdateDimensions, and requests a decorView relayout. Same holder-reseeding approach as the DeX work in fix(mobile): keep Android layout intact in Samsung DeX / external-display windows #16018, without the DeX-specific manifest meta-data; unlike it, the screen holder also takes windowed metrics, which the text-measure pipeline requires.densityto the activityconfigChanges, so the pane density switch reaches the hook instead of recreating the activity.Verification
Samsung foldable (One UI, Android 16): with the app update-installed and brought through recreation-free fullscreen ↔ split-pane transitions in both directions, layout and text fill the pane correctly in split panes on both displays as well as pop-up windows; full-screen behavior unchanged (both holders equal the display metrics there). The
fullUserorientation plugin is untouched. JS-side layout code needed no changes.Co-Authored-By: CCCha [email protected] via Claude Code