Repository navigation
fix(mobile): repaint the picked language row in the language picker - #8765
Conversation
The sheet's FlashList only repaints a mounted cell when its data or extraData changes identity, and the row received the selection through a renderItem closure. Pass the selection and the in-flight busy state through a memoized extraData, hoist renderItem, and memoize the row on its own selected flag so a tap repaints only the rows that changed.
iscekic
added
drain
|
bot: Android emulator (1080x2400, CI dev-client APK, this branch's JavaScript through Metro), fresh local dev stack with no signed-in account. Exercised on the login screen: Language control → search the language → tap the row → Done.
|
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Executive SummaryI re-verified the whole diff at Assumptions: Overview
Both earlier suggestions are resolved at this head: Files Reviewed (19 files)
Previous Review Summaries (5 snapshots)Current summary above is authoritative. Previous snapshots are kept for context only. Previous reviewStatus: 1 Suggestion Found | Recommendation: Address before merge Executive SummaryThe incremental commit fixes the earlier Retry gap: Assumptions: Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (3 files)
Fix these issues in Kilo Cloud Previous review (commit 975ade2)Status: 2 Suggestions Found | Recommendation: Address before merge Executive SummaryThe three new commits resolve both earlier suggestions and add real test coverage for the startup-direction branches; the remaining gaps are the shared Retry for the new Assumptions: Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (17 files)
Fix these issues in Kilo Cloud Previous review (commit 1d49990)Status: 3 Suggestions Found | Recommendation: Merge Executive SummaryThe new Assumptions: Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (5 files)
Previous review (commit 6f5321a)Status: 2 Suggestions Found | Recommendation: Merge Executive SummaryThe picker repaint change is unchanged and remains sound; the new direction-attempt guard is logically consistent (one automatic relaunch per language, then the retry screen), with two non-blocking gaps: the apply path ignores a failed attempt record and the Assumptions: Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (8 files)
Notes: Previous review (commit b2fae4d)Status: No Issues Found | Recommendation: Merge Executive SummaryThe change correctly moves the picker selection and busy state into a memoized Assumptions: Files Reviewed (4 files)
Notes: the Reviewed by deepseek-flash-latest · Input: 115.9K · Output: 28.7K · Cached: 1.5M Review guidance: REVIEW.md from base branch |
iscekic
added
merge:blocked:insufficient-e2e
The cold start forced the native layout direction and relaunched whenever the running direction did not match the stored language. A relaunch is the only way to apply a forced direction, but it is not guaranteed to come back with it: the app then relaunched again on every launch and stayed on a blank frame, and the chosen language never reached the screen. Record the language whose direction the last launch relaunched for, and refuse a second automatic relaunch for the same language. The app shows its existing retry screen instead, whose retry clears the record so the user can ask for the attempt again. The picker's own apply path writes the same record before it relaunches, so it cannot feed the loop either.
iscekic
removed
merge:bot
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
iscekic
added
merge:blocked:insufficient-e2e
expo-localization reset the React Native RTL preferences from its build-time template values on every React instance, and this app does not set those values: the unset string resolves to the template default, so each new instance forced the direction back to LTR before React rendered. An app-chosen RTL language then never took effect, and the cold-start relaunch repeated on every launch. Leave the preference alone when the template value is unset on both platforms; an explicitly configured value still applies.
iscekic
removed
merge:bot
iscekic
added
merge:blocked:insufficient-e2e
The picker's apply path wrote the relaunch record and reloaded regardless of the write result, so a failed write still relaunched and the next launch had no record to refuse a second attempt. Honour the documented contract: when the record cannot be written, keep the current direction and show the picker's saved-language retry state instead of reloading.
Move the direction step's effects out of the root layout into one function with injected effects, so the branches that decide between an in-place start, one relaunch, and the retry screen are covered: a missing record, a refused record write, a failed reload, and the retry control dropping the record first.
The package publishes a prebuilt AAR, and Gradle links it unless the module is listed for a source build, so the layout-direction guard this branch patches into the module's Kotlin never compiled on Android. List the module for a source build and record the coupling beside the patch entry.
iscekic
removed
merge:bot
iscekic
added
merge:blocked:kilobot-not-ready
One Retry served two different restart failures. A failed reload had already forced the direction and written the attempt record, so reloading again is exactly the step to repeat. A skipped restart never reached either: the record write failed, so reloading there came back in the previous direction with still no record, and the direction then needed a second restart to arrive. Keep which step failed and re-run the apply path for the skipped one, so Retry retries the record write and only then relaunches. The mounted test drives the skipped case through Retry and asserts the second write, the forced direction and one reload.
iscekic
added
merge:blocked:kilobot-not-ready
|
bot: Runnable proof of the language picker on both platforms, from the login screen with no account signed in. Android emulator, dev client built from this branch:
iOS simulator, dev client built from this branch: A cold start of each app comes back on the same Arabic screen without another relaunch.
|
iscekic
removed
merge:blocked:insufficient-e2e
Evidence
Evidence: demonstrated — on the pre-fix build the picker's choice never reached the screen for an RTL language: the preference persisted and the native relaunch started, but the app came back in the previous direction (the earlier run on this dev client recorded the frame staying blank while the entry point re-ran). With this branch, on an Android emulator dev client built from this branch and on an iOS simulator build from this branch, tapping the Arabic row moved it from "radio button, unchecked" to "radio button, checked", and Done relaunched the app once into the Arabic login screen with a right-to-left layout, which a cold start keeps.
What changed
The selection reaches the list rows
The picker's list only repaints a mounted cell when one of its comparison
inputs changes identity. The sheet handed the row its selection through an
inline
renderItemclosure and gave the list noextraData, so the repaintdepended on that closure being recreated on every parent render: any hoist of
renderItemwould silently leave a tapped row showing the previous check markand accessibility state, while every parent render still re-rendered all
mounted rows.
language-picker-sheet.tsx: pass the selection and the in-flight apply stateto the list as a memoized
extraData({ selected, disabled: busy }), andhoist
renderItemin auseCallback.language-picker-row.tsx: takeselectedas the row's own boolean and wrapthe row in
React.memo, so a selection change repaints only the row thatlost the check mark and the one that gained it.
The forced direction survives the relaunch
A relaunch is the only way to apply a forced native direction, and the app was
relaunching for it on every launch: the direction the picker forced was reset
by the localization module's template defaults before React rendered, so the
cold start saw the previous direction again and relaunched. The app never
painted in the chosen direction, and a launch that came back wrong repeated
forever.
patches/expo-localization@58.0.4.patch: the module only writesallowRTL/forceRTLwhen the merged Android string resource (or the iOSInfo.plist key) actually carries a boolean. Both packaged resources are the
literal
unset, and the app does not configure the config plugin, sotoBooleanStrictOrNull()returnsnulland the app stays the only writer ofthe direction.
apps/mobile/package.json: listexpo-localizationunderexpo.autolinking.android.buildFromSource. Android links the package'spublished AAR unless the module is listed, so without this the patched Kotlin
never compiled, and the coupling is recorded beside the patch entry in
pnpm-workspace.yaml.i18n/direction-attempt.ts: record the language whose direction the lastlaunch relaunched for, and clear it once the direction matches.
i18n/startup-direction.tsandi18n/startup-direction-run.ts: the coldstart's decision (apply in place, relaunch once, or show the retry screen) and
the effects that run it. A second automatic relaunch for the same language is
refused, so a direction that never takes effect cannot loop; the retry control
drops the record first, because a retry is the user asking again.
i18n/apply-language.ts: write the record before the picker's own relaunch,and report
restart-skippedwhen the record could not be written.language-picker-sheet.tsx: keep which step failed. Retry reloads after afailed reload, and re-runs the apply path after a skipped restart, so the
record write is retried instead of reloading into the same refused launch.
Tests
language-picker-sheet.mounted.test.tsx: a tap marks the row selected, thelist's
extraDatamoves to the tapped language, Done applies it, and Retryafter a skipped restart re-runs the apply path (record written twice, direction
forced once, one reload). The retry assertion fails without the change.
startup-direction-run.test.ts: the apply, relaunch, refused second relaunch,write-failure and failed-relaunch branches, and that the retry drops the
record before reloading.
apply-language.test.ts: the record is written before the relaunch and afailed write skips it.
End-to-end verification
VM: fresh drain work VM. Android: local dev stack, emulator
omp36(defaultdensity and font scale), CI dev-client APK built from this branch's head
(
mobile-native-android-a338432871d9f43f201e0315bb0e22f2806223a2, so the modulecompiled from source with the patch) and Metro serving this branch.
Android flow, from the login screen:
arabic→ the row reportedchecked="false"; after the tap the row's radio node reportedchecked="true".مرحبًا بك في Kilo,عنوان البريد الإلكتروني,المتابعة عبر البريد الإلكتروني.(leading text rightmost, then
الشروط,و,سياسة الخصوصية, and the finalstop leftmost), the sheet's header put Cancel on the right and Done on the
left, and
RCTI18nUtil_forceRTL=trueis persisted after the relaunch.am force-stop, relaunch) came back on the same Arabic screenwithout another relaunch and without the retry screen.
English→ Done relaunchedonce into the English login screen.
iOS:
ios_appreused themobile-native-buildartifact of this checkout'snative fingerprint (
7126375e…, simulator), Metro and the backend on the lane'sport offset, driven through the lane's agent-device session on the login screen:
arabic→ the cell's accessibility value readradio button, uncheckedbefore the tap andradio button, checkedafter it.مرحبًا بك في Kilo(بالمتابعة، توافق على الشروط و سياسة الخصوصية.,المزيد من خيارات تسجيل الدخول).edge) and Done at x≈21 (left edge), the mirror of the English sheet.
Checks:
vitestfor the picker, row, startup-direction, direction-attempt andapply-language suites;
tsgo --noEmit,oxlintandoxfmt --list-differentfor
apps/mobile.Production prerequisites
None.
Proxy answers
The owner's proxy gave these answers in the owner's place.
Answer: Memoized row + boolean selected + memoized extraData (Recommended)
Confidence: 0.80 · Precedent: Engineering conventions / Performance reviewer: 'check performance on large tables and hot paths'. The brief asks for bounded row re-renders, and this option follows the repo's list convention.
Answer: Yes, include disabled in extraData (Recommended)
Confidence: 0.85 · Precedent: Fix the class of the issue: 'if you saw a gap, fix it'; 'fix class of issue'.
Answer: Attempt an ios_lane lease anyway
Confidence: 0.75 · Precedent: Verify on both platforms: 'Both iOS and Android need to be verified'. Agent requests the iOS lane: 'iOS lane must specifically be asked for by the agent'. The brief names ios_lane explicitly. If the lease is refused, treat the refusal as a workflow bug, not as an end state.
Answer: Extend language-picker-sheet.mounted.test.tsx (Recommended)
Confidence: 0.80 · Precedent: Least work, no drop in quality: 'do the least amount of work possible, with no drop in quality'. Reuse the existing mock harness instead of duplicating it.
RCTI18nUtil_forceRTLwas written. How should this PR handle it?Answer: Fix it in this PR. The brief requires Arabic or Urdu chosen from login to reload into an RTL layout, and a permanent reload loop bricks the app. Find the root cause of the direction not surviving. First rule out a dev-client artifact with a local release build, then fix the real cause. Also add the bounded guard: at most one direction reload per launch, then fall back to the existing Retry/Continue screen, so the app can never loop. Prove Arabic and Urdu end to end on Android and on iOS (ios_lane) from the login screen.
Confidence: 0.60 · Precedent: Autonomy: 'Fix the root cause and the class of the issue'.
Answer: File a follow-up work item with the measured evidence
Confidence: 0.55 · Precedent: Product scope: 'Send every work item through triage, including manual items'.
Answer: Implement the native restart in this PR
Confidence: 0.55 · Precedent: Autonomy: fix the root cause and the class of the issue, and never accept blocked as an end state. Proof: build native iOS in CI.