Repository navigation
fix(mobile): keep the app responsive when Back interrupts the folder picker opening - #8076
Conversation
Picker sheets (formSheet routes) had no hardware Back listener, so a Back that landed during the sheet's opening transition raced the native BottomSheetBehavior dismissal instead of popping through the same JS path the header controls use. PickerSheet now arms one hardwareBackPress listener for its mount lifetime. A focus-aware guard collapses the double-dismissal race (native hide triggers a JS pop; a second pop is a no-op on an unfocused route). The default dismiss calls router.back() directly - never onDone, which several sheets use to commit a choice (save, apply, reload). Sheets that consume Back internally pass onHardwareBack (return true when handled); the folder picker wires it to its drill stack so hardware Back walks the in-sheet directory stack first and dismisses at the launch frame.
iscekic
added
drain
|
bot: Verified on the Android 16 emulator with the fix loaded from Metro. Exercised: sign-in with a test account, connect a test CLI instance, type a draft prompt, then tap the Folder field and press system Back while the sheet was still opening (repeated across 0-250 ms offsets). The first capture shows the sheet mid-open with the real directory listing; the second shows the new-session form restored right after the interrupt, with the typed draft still present.
|
|
bot: Second surface, same loop: the Run on (instance picker) sheet was opened and system Back was pressed at 20/80/150/250 ms offsets, three attempts per offset. After every attempt the app stayed foreground and responsive, the new-session form returned with the instance still selected, and
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Executive SummaryThe only change since the previously reviewed commit is a formatting-only reformat of the Assumptions: Files Reviewed (1 file)
Previous Review Summaries (6 snapshots, latest commit 2849283)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 2849283)Status: No Issues Found | Recommendation: Merge Executive SummaryThe incremental head adds two harness-only fixes (a Assumptions: Files Reviewed (12 files)
Previous review (commit ec98b4c)Status: No Issues Found | Recommendation: Merge Executive SummaryThe incremental commit adds one focused mounted suite for the mode picker's hardware Back path; its mocks match every import the real route and Assumptions: Files Reviewed (1 file)
Previous review (commit 970f82b)Status: No Issues Found | Recommendation: Merge Executive SummaryThe incremental commits only fold the duplicated Assumptions: Files Reviewed (11 files)
Previous review (commit 6289bd8)Status: No Issues Found | Recommendation: Merge Executive SummaryThe incremental commit removes Files Reviewed (7 files)
Verification note: read-only, non-executing review. Dependencies are not installed in the sandbox, so the new deep import ( Previous review (commit d4e433a)Status: 1 Issue Found | Recommendation: Address before merge Executive SummaryThe incremental change only moves the hardware-Back tests into a dedicated file; the previously reported CRITICAL mock gap in four suites that render the real Overview
Issue Details (click to expand)CRITICAL
Files Reviewed (2 files)
Verification note: read-only, non-executing review. The incremental diff ( Fix these issues in Kilo Cloud Previous review (commit 4a2f049)Status: 1 Issue Found | Recommendation: Address before merge Executive SummaryThe new hardware-Back listener makes Overview
Issue Details (click to expand)CRITICAL
Files Reviewed (4 files)
Verification note: this was a read-only, non-executing review; the broken-suite finding comes from static inspection of the affected mocks, so tests were not run. Reviewed by deepseek-flash-latest · Input: 31.6K · Output: 1.9K · Cached: 209.8K Review guidance: REVIEW.md from base branch |
… file The body suite grew past the repo's 300-line file limit; the hardware Back suite moves to folder-picker.back.mounted.test.tsx with the same harness.
useNavigation() throws outside a navigator and breaks harnesses that mock expo-router partially. Reading focus through the vendored react-navigation context (deep-imported like prevent-remove.ts) returns undefined instead, and the Back listener degrades to nothing there. Accessing BackHandler also moves inside a guard: the vitest module mock throws on the property access itself when the export is absent. The hardware Back suite provides the context so the listener arms there; four sheet harnesses and the excluded-feature chat sheet mount unchanged.
|
bot: Addressed in 6289bd8. The shell no longer calls |
iscekic
added
merge:human
…ack-responsive # Conflicts: # apps/mobile/src/components/agents/instance-picker.mounted.test.tsx # apps/mobile/src/components/app-unlock-screen.test-helpers.tsx # apps/mobile/src/components/picker-sheet.tsx
…ack-responsive # Conflicts: # apps/mobile/src/components/picker-sheet.tsx
iscekic
removed
merge:bot
…ack-responsive # Conflicts: # apps/mobile/src/components/todos/todo-list-model-picker.mounted.test.tsx
iscekic
added
merge:blocked:insufficient-e2e
|
bot: Re-ran the Android proof on the current head with the dev client loaded from Metro on this VM, a fake account signed in through the app's email code, and a fake CLI instance answering a real directory listing, so the folder picker rendered its own rows. Observed, in order:
Captures below: the sheet with its listing rows, then the form after the 12 folder-picker interrupts, then the form after the instance-picker interrupts. The screen text was read back with an independent OCR pass, and the PNGs carry no text metadata chunks.
|
|
bot: Before/after on the same emulator, same build of the app's native shell, same signed-in test account, and the same computer connection; only the JavaScript bundle changed. Before (main at the branch's merge base, loaded from a second Metro on the same VM): open the folder picker, drill into a folder, press system Back once. The picker closes completely and the new-session form comes back. After (branch head): the identical sequence keeps the sheet open and shows the launch folder's listing again, so Back walked one level up inside the picker; a further Back returns to the form. Also on the head: tapping the Folder row and pressing Back while the sheet is still opening returns to the form with the typed draft intact. For the opening-transition race itself the two builds do not differ consistently: on the base build the same press sometimes left the sheet open and sometimes closed it, and the originally reported freeze did not occur on either build. The difference the change guarantees is which layer handles Back, which is what the drill comparison and the interrupt loops show.
|
picker-sheet.tsx kept both sides: the sheet still reads its route's focus through `useRouteFocused` for the expired-shell guard that main added, and the hardware Back listener keeps its own press-time focus read from the same navigation context, so the listener still degrades to nothing where no navigator is above the shell. Every hook stays above the expired-shell early return.
…mport The expired-shell suite mounts the shared sheet shell, so its router mock now answers the shell's dismiss path, and the folder Back suite's translation mock accepts the plugin install that the real i18n module performs at import time.
iscekic
removed
merge:bot
The merged focus read fits on one line under the repository formatter.
|
bot: Re-ran the interrupt on the current head after merging main. Dev client from Metro on the local stack, an owned test account, and the published CLI running as the connected computer that answers the directory listing. Exercised: typed a draft, selected the connected computer, tapped the folder field, then pressed system Back at 0/80/160/280/450 ms, three attempts per offset. All 15 attempts returned to the new-session form with the draft and the folder row intact, no sheet left on screen, and no application ANR lines in The first capture is the sheet open with the directory rows the connected computer returned. The second is the new-session form right after the interrupt, with the typed draft still there. The third is the folder row after the interrupt, still naming the launch folder — Back never committed a choice. The drill step was also re-checked both ways: on this head, Back inside a subfolder returned to the launch listing with the sheet open and a second Back closed it; with the pre-fix shell staged in the worktree, one Back inside the same subfolder closed the picker outright.
|
|
bot: Re-ran the same flow on the final head (e00a47b) after the formatter commit, so the published proof and the head match exactly. Exercised: same stack and device flow — a typed draft, the connected computer selected, the folder field tapped, then system Back at 0/80/160/280/450 ms, three attempts per offset. 15 of 15 attempts returned to the new-session form with the draft and the folder row intact; no application ANR lines in The first capture is the sheet open with the directory rows the connected computer returned; the second is the form after the interrupt with the draft still present.
|
iscekic
added
merge:blocked:insufficient-e2e
|
bot: Re-ran the flow on the current head on the Android emulator of the work VM: dev stack, dev client loaded from Metro, an owned test account, and the published CLI as the remote instance that answers the directory listing. Exercised: typed a draft, selected the connected computer, opened the folder sheet, then pressed system Back at 0/80/160/280/450 ms, three attempts per offset — 15 of 15 returned to the new-session form with the draft and the folder row intact. The same loop on the run-location sheet held on 15 of 15. Drilling into a subfolder and pressing Back once returns to the launch folder with the sheet still open; a second Back closes the sheet and keeps the draft. With the pre-change sheet staged in the same worktree and the app relaunched onto that bundle, one Back inside the subfolder closed the whole picker. Captures: the folder sheet inside the subfolder; the sheet back at the launch folder after one Back; the new-session form with the draft after the sheet closed; and the pre-change build after the same single Back, where the picker is already gone.
|
|
bot: Hardware Back around the folder picker on the Android dev client at this head, with the picker's listings coming from the local CLI remote's project folder (the two child folders exist only there).
Limitation: on the head the press sometimes lands before the folder route mounts, and the platform sheet then dismisses itself; both outcomes keep the prompt, the selected computer and the folder row, and the end state is stated per attempt in the pull request body rather than asserted as one.
|
|
bot: Hardware Back on the folder picker, driven on the Android emulator of the work VM against this head. The app is the dev client loaded from Metro on the local dev stack, signed into an owned test account, and the folder listings come from the published CLI running as the connected computer — the listed folders exist only in that CLI project, so the sheet is showing a real remote listing. Observed with
|
iscekic
removed
the
merge:blocked:insufficient-e2e
Evidence
Evidence: demonstrated — re-ran the end-to-end proof on head
e00a47bon this VM's Android emulator against the local dev stack: the dev client loaded from Metro, an owned test account signed in through the stack's email-code flow, and the published CLI running as the connected computer whose project folder supplied the folder listings. Hardware Back pressed while a picker sheet was opening returned to the new-session form with the draft, the computer and the folder row intact on every attempt, and the drill case shows the changed behavior on the same device and flow: at the head one Back pops one folder level and keeps the sheet open, where the same flow with the branch's sheet shell replaced by main's closes the whole picker.The captures from this run are in a
bot:comment on this pull request; each was taken together with theuiautomatordump that names the state.Change
Picker sheets (the app's native
formSheetroutes) had no hardware Back listener, so a Back press that landed while a sheet was still opening raced the native bottom-sheet dismissal instead of popping through the same JavaScript path the header controls use.PickerSheetarms onehardwareBackPresslistener for its mount lifetime (Android; RN ships BackHandler as a no-op on iOS, so one implementation serves both platforms).@/lib/navigation/navigation-context), notuseNavigation(): a surface outside a navigator, and a harness with a partial expo-router mock, then mounts unchanged and the listener degrades to nothing there.router.back()directly — neveronDone, which several sheets use to commit a choice (save, apply, reload).onHardwareBack(returntruewhen handled). The folder picker wires it to its drill stack: Back walks the in-sheet directory stack first and dismisses the sheet at the launch frame.End-to-end verification
Real surface at head
e00a47b, on this VM: Android emulator, the app's dev client over Metro, the local dev stack for API and auth, and the published Kilo CLI as the connected computer. The picker listeddocs,srcandtests, which exist only in that CLI project, so the listing came from the remote rather than from a fixture.srctapped: one Back restored the root listing with the sheet still open — the capture at that step is byte-identical to the root capture, so the frame was popped in place — and a second Back closed the sheet. The draft was present in the form at both steps.picker-sheet.tsxreplaced by main's in the same worktree (Metro restarted, app relaunched, same account and same CLI remote), the same nested flow closed the whole picker on the first Back. Restoring the branch file and repeating the flow popped one drill frame instead.adb shell dumpsys activity lastanrreports "no ANR has occurred since boot" after all runs,adb logcat -dholds no "ANR in", "Application Not Responding" or "Input dispatching timed out" line, and the app window stayed focused.Scope notes, so the coverage is not overstated:
Merge resolution (main through 6a88e76)
PickerSheetreads focus twice on purpose:useRouteFocused()supplies the render-time value for main's expired-shell guard, while the Back listener reads the same context at press time; every hook sits above the early return, so hook order is stable.onHardwareBackalongside main'stopInsetanddoneDisabledprops; oneexpo-routermock in the sheet-header harness; main's ownBackHandlerstub in the to-do list model picker harness.Production prerequisites
None.
Proxy answers
The owner's proxy gave these answers in the owner's place.
Answer: All PickerSheet routes
Confidence: 0.80 · Precedent: Fix the root cause and the class of the issue on every affected surface: the owner precedent, "fix this for all shared services model pickers on all surfaces"; non-repro → ship the reasonable fix: the owner precedent, "Merge fix(mobile): polish quick chat composer edge and session list loading rows #7613 and close this"
Answer: No (Recommended)
Confidence: 0.60 · Precedent: Put each separate concern in its own PR: the owner precedent, removal of the frontier flag "in a separate PR"; do the least work possible: the owner precedent
Answer: Inside PickerSheet (Recommended)
Confidence: 0.80 · Precedent: Fix the root cause and the class: "fix this for all shared services model pickers on all surfaces" (owner precedent)
Answer: Walk drill stack first
Confidence: 0.55 · Precedent: Keep similar elements consistent: owner asked that "the thought row match the tool call row" (owner precedent). Hardware Back should behave like the in-sheet Back control.
Answer: Focus-aware guard
Confidence: 0.55 · Precedent: Fix the root cause and the class: "fix class of issue preventing progress" (owner precedent). A second router.back after the sheet has already popped can pop new.tsx and lose the draft, which is the reported bug. Guard on focus and prove the double-Back case on the emulator.
Answer: onHardwareBack prop (Recommended)
Confidence: 0.75 · Precedent: Engineering conventions: extract shared code into an internal library rather than couple it (the owner precedent, "extract any common code to an internal library"); the prop keeps the shared shell generic so every sheet gets the fix.
Answer: Loop on two sheets
Confidence: 0.55 · Precedent: Autonomy: fix the class of the issue on every affected surface (the owner precedent, "fix this for all shared services model pickers on all surfaces"). The shared PickerSheet change affects every sheet, so the proof should cover a second sheet as well as the folder picker.
Answer: Defensive hooks (Recommended )
Confidence: 0.60 · Precedent: Product scope: the excluded features stay untouched — the owner precedent, whose routes are end-of-life and out of scope; plus least work: "do the least amount of work possible, with no drop in quality"
queuedcheck on that head at 16:09 and never started it; I checked 10 other open PRs whose head is a merge commit and none of them has a Kilo Code Review check at all, so the review app appears to skip merge commits. The host end gate needs a completed kilobot review of the head, and pushing an empty commit to trigger reviews is forbidden. How should I get a reviewable head?Answer: Add a real test commit
Confidence: 0.75 · Precedent: Never accept blocked as an end state; the item must converge (owner precedent: "a work item must converge"; owner precedent: "blocked is not an acceptable end state")
Answer: Wait for the host judge
Confidence: 0.60 · Precedent: Speed and least work: 'skip work that doesn't need to be done, do not redo work that was already done' (owner precedent); an extra commit only to retrigger the rejudge reruns all 100 checks for no quality gain
Answer: Coverage list plus before/after
Confidence: 0.60 · Precedent: Proof and verification: 'only bug reports technically need to care about repro' (owner precedent). This is a bug report whose brief asks to reproduce on the emulator, so a pre-fix run is needed. Rewriting the coverage list costs little and targets the judge's 'does not cover the change' finding.