Repository navigation
fix(mobile): keep centered states inset on both sides in RTL - #8264
Conversation
A centered state passed its horizontal inset through className, which styles the scroller itself: the scroll content keeps its own width, so the end side stays flush in a right-to-left layout. CenteredState now takes contentContainerClassName and every padded caller uses it.
A scroll view that sets contentContainerStyle drops the caller's contentContainerClassName, so CenteredState applies the inset to the box that holds its children instead. The inset then narrows the content on both sides, in either layout direction.
iscekic
added
drain
|
bot: Verified on the Android emulator (1080x2400, 420 dpi, light theme) against the local dev stack with an owned fake account, on the head commit of this branch. Both captures show the code reviewer GitHub connect gate. The card measures 55 px (21.0 dp) from each screen edge in both directions, and the connect button measures 110 px (41.9 dp) from each edge: the card's own inset plus the content-box inset.
The Android emulator also rendered the previous scroller padding symmetrically, so these images show the required state rather than a before/after difference on this platform; the change removes the platform-dependent inset path itself.
|
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Executive SummaryThe mobile centered-state inset fix is consistent across all 12 padded callers, the shared component keeps scroller styling separate from content-box insets, and the added mounted assertion pins the placement with no memory, lifecycle, or test-teardown concerns. Files Reviewed (14 files)
Reviewed by deepseek-v4.1-flash-1017 · Input: 60.5K · Output: 11.5K · Cached: 867.2K Review guidance: REVIEW.md from base branch |
iscekic
added
merge:bot
What changed
A centered state passed its horizontal inset through
className, which styles the scroller itself. The scroll content keeps its own width and placement, so the inset never narrowed the content: in a right-to-left layout the end side stayed flush, the card's rounded corners were clipped at the screen edge, and its text and button sat off centre.apps/mobile/src/components/centered-state.tsxtakes acontentContainerClassNameprop and applies it to the box that holds the state's children. A scroll view that sets its owncontentContainerStyledrops a caller's content-container class (the reasonTabScreenScrollViewuses a trailing spacer instead), so the class lands on the content box, where the padding narrows the content on both sides in either direction.classNamekeeps the scroller's own styling: background and width.classNamemoved it tocontentContainerClassName: the code reviewer GitHub and Bitbucket gates, the review spectator copy, the three PR review reconnect notices (discussion tab, file navigator, diff file list), the agents file and tool sheets, the share destination list, and the member-limit sheet.centered-state.mounted.test.tsxpins that the inset lands on the content box and not on the scroller.apps/mobile/AGENTS.mdrecords the rule in one line.End-to-end evidence
Android emulator (omp36, 1080x2400, 420 dpi, light theme), local dev stack on this VM, fake account, deep link to the code reviewer GitHub connect gate. Card insets measured from the screenshot's card fill (
bg-secondary), in device pixels:mainmainThe inset now comes from the content box, and the measurement proves it: with the caller's class set to
px-12the same screen measured 41.9 dp on each side (2 x 21 dp, the content-box padding stacking with the card's ownp-6), and the connect button measured 110 px (41.9 dp) from each screen edge.The Android emulator rendered the old scroller padding symmetrically, so these captures show the required state rather than a before/after difference on this platform. The platform-dependent mechanism is gone: no centered state applies an inset to a scroller style any more. iOS proof needs a lane and this change is not iOS-specific (no iOS-only code, no
Platform.OS, keyboard, safe-area or truncation lines), so no lane was leased; the one iOS surface this touches is measured on Android in both directions.Screenshots are attached in a comment below.
Checks
pnpm --filter kilo-app test: 1194 files, 17899 tests passed, 2 skipped.pnpm --filter kilo-app typecheck,lint,format:check,check:classes: clean.Proxy answers
The owner's proxy gave these answers in the owner's place.
Answer: Add contentContainerClassName
Confidence: 0.85 · Precedent: Fix the root cause and the class of the issue when the same mistake repeats; it also matches the repo's existing ScrollView convention.
Answer: All 12 padded callers
Confidence: 0.93 · Precedent: Fix the root cause and the class of the issue on every affected surface, not just the reported one.
Answer: Yes, one mounted assertion
Confidence: 0.70 · Precedent: Keep CI checks that catch real errors in PRs as early as possible.
Answer: Yes, one line
Confidence: 0.70 · Precedent: Record discoveries for future agents, briefly.
The prop name follows the answer; its target is the content box rather than the scroll view's
contentContainerStyle, because that style is already set by the centering layout and drops the caller's class.