Skip to content

fix(mobile): seed open soft keyboard height when mounting TextInputModal - #32027

Closed
zhangxaochen wants to merge 4 commits into
stablyai:mainfrom
zhangxaochen:zhangxaochen/mobile-rename-modal-seed-keyboard
Closed

zhangxaochen wants to merge 4 commits into
stablyai:mainfrom
zhangxaochen:zhangxaochen/mobile-rename-modal-seed-keyboard

Conversation

@zhangxaochen

Copy link
Copy Markdown
Contributor

ELI5

On Orca Mobile, when the soft keyboard is already open from focusing a terminal input field, double-tapping a terminal tab now opens the Rename Terminal modal cleanly above the keyboard instead of behind it.

What Changed

  • Before: When the soft keyboard was already open (currentSoftKeyboardHeight() > 0), double-tapping a terminal tab in MobileSessionTabStrip opened TextInputModal ("Rename Terminal") at the bottom of the screen behind the open keyboard.
  • After: TextInputModal immediately seeds the existing soft keyboard height when mounting and renders above the open keyboard.
  • Mechanism:
    • Added an opt-in seedKeyboard?: boolean prop (default false) to BottomDrawer and MountedBottomDrawer so content-sized drawers (fillAvailable = false) that open without dismissing the keyboard can seed currentSoftKeyboardHeight() on mount.
    • Enabled seedKeyboard on TextInputModal (<BottomDrawer visible={visible} seedKeyboard ...>), while leaving seedKeyboard off by default so drawers like NewWorktreeModal that call Keyboard.dismiss() before opening do not flash at a stale keyboard height during the hide transition.

Why

MobileSessionTabStrip uses useKeyboardPersistingTaps('handled'), which preserves focus on the active terminal input when tapping tabs so the soft keyboard never closes. When TextInputModal mounts, its <TextInput autoFocus /> transfers focus directly from the terminal input without changing the keyboard height, so subscribeSoftKeyboard fires no new onShow event. Seeding currentSoftKeyboardHeight() on mount via seedKeyboard lifts TextInputModal immediately without regressing content-sized sheets that dismiss the keyboard first.

Linked Issue

Fixes #32025

Visual Proof

N/A - Verified on a physical Android phone (vivo V2602A) via ADB (dumpsys input_method mInputShown=true + screenshot verification) and covered by unit tests.

Testing

  • I manually tested these changes locally
  • Automated tests added/updated, or explained why not below
  1. mobile/src/components/mounted-bottom-drawer-keyboard-seam.test.tsx: Verifies that a content-sized BottomDrawer with seedKeyboard seeds currentSoftKeyboardHeight() on mount while the default seedKeyboard=false remains unseeded.

AI Disclosure

Assisted by Google DeepMind Antigravity agentic coding assistant for implementation and test coverage.

Review

Agent skill upstream boundary

  • Not applicable, or this change copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

Ensure no issues in: Security, Cross-platoform support (Linux, Windows, Mac), Remote SSH, Mobile, general backwards compatibility, performance

Checklist

  • This PR is small and focused
  • I explained what changed and why (ELI5, the user-facing before/after, the mechanism, and why over the alternatives)
  • Before/after screenshots or videos attached for UI changes, or N/A with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
  • pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred)

@coderabbitai

coderabbitai Bot commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Explain Change →

Walkthrough

The bottom-drawer path adds an optional seedKeyboard prop. MountedBottomDrawer defaults the prop to false and seeds the current keyboard height for content-sized drawers when it is enabled. TextInputModal passes the prop to BottomDrawer. Tests cover initial seeding, keyboard show and hide updates, timing fallbacks, and subscription cleanup.

Severity of issue fixed: Medium

Merge Risk: Low · up to d6d57

Some iOS keyboard transitions may misposition the rename prompt. Correct the tests and track these edge cases.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: seeding the open soft keyboard height when mounting TextInputModal.
Description check Passed The description includes the required sections and clearly explains the user impact, implementation, rationale, linked issue, visual proof, testing, risk considerations, and checklist status. The omit…
Linked Issues check Passed Issue #32025 is closed and supplies historical context only. No active directly linked issue target remains, so no linked-issue coding requirement applies. The PR changes remain consistent with the de…
Out of Scope Changes check Passed The PR changes are limited to keyboard-height seeding for bottom drawers, enabling the option in TextInputModal, and tests for the related behavior. The default remains false, which preserves existing…
Full details: Description check

Explanation

The description includes the required sections and clearly explains the user impact, implementation, rationale, linked issue, visual proof, testing, risk considerations, and checklist status. The omitted test anti-pattern checkbox is non-critical because automated test coverage is described.

Full details: Linked Issues check

Explanation

Issue #32025 is closed and supplies historical context only. No active directly linked issue target remains, so no linked-issue coding requirement applies. The PR changes remain consistent with the described historical fix and add focused automated coverage.

Full details: Out of Scope Changes check

Explanation

The PR changes are limited to keyboard-height seeding for bottom drawers, enabling the option in TextInputModal, and tests for the related behavior. The default remains false, which preserves existing drawer behavior. No unrelated change is demonstrated.

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 4 functions across 4 files.

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

Share

@coderabbitai coderabbitai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


Review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 88c65a84-5f15-456d-9837-ca754d5c58e9
Commits

Reviewing files that changed from the base of the PR and between a424c69 and 2eed02e.

Files selected for processing: 3
  • mobile/src/components/BottomDrawer.tsx
  • mobile/src/components/TextInputModal.tsx
  • mobile/src/components/mounted-bottom-drawer.tsx

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review.

@coderabbitai coderabbitai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up Comments

Outside diff Limit the content height above the keyboard.

275-276: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Limit the content height above the keyboard.

When a landscape viewport has less space above the keyboard than the prompt needs, seedKeyboard translates the content-sized drawer upward without reducing its content cap. The drawer top can move above the visible area, and the ScrollView cannot scroll downward to reveal content above its top edge.

Reduce the content cap by keyboardInset so the prompt remains scrollable within the keyboard-visible area.

Suggested fix
  const contentCap =
-    screenHeight - insets.top - spacing.lg - (insets.bottom + spacing.lg) - HANDLE_AREA_HEIGHT
+    screenHeight -
+    insets.top -
+    spacing.lg -
+    (insets.bottom + spacing.lg) -
+    HANDLE_AREA_HEIGHT -
+    keyboardInset

Open the source location

// content-sized sheet seeds only when it opts in. Page sheets on web seed
// because focus moving under an open IME sends no event; native content
// sheets own a Modal window, where stale metrics would stick.
if (fillAvailable || seedKeyboard || Platform.OS === 'web') {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor · Functional Correctness · Quick win

Supported by static analysis

Script executed:

set -eu
printf '%s\n' '--- drawer file ---'
git show be4beeb57ced2c8b19489f1127dc664f739ee08a:mobile/src/components/bottom-drawer/mounted-bottom-drawer.tsx | nl -ba | sed -n '1,250p'
printf '%s\n' '--- keyboard seam candidates ---'
rg -n -F --glob '*.ts' --glob '*.tsx' -e 'Keyboard.addListener' -e 'Keyboard.metrics' -e 'keyboardWillChangeFrame' -e 'keyboardDidShow' mobile/src mobile/app || test "$?" -eq 1
printf '%s\n' '--- modal and drawer seed references ---'
rg -n -F --glob '*.ts' --glob '*.tsx' -e 'seedKeyboard' -e 'TextInputModal' -e 'Rename Terminal' mobile/src mobile/app || test "$?" -eq 1
printf '%s\n' '--- changed diff ---'
git diff --no-ext-diff --unified=80 1b15d45a5ecde93e7433ccf8debc63e8d18253e6 be4beeb57ced2c8b19489f1127dc664f739ee08a -- mobile/src/components/bottom-drawer

Repository: stablyai/orca

Length of output: 38771


Web query:

React Native 0.83.10 Keyboard.metrics keyboardDidShow keyboardWillChangeFrame implementation

Result:

Sources consulted:
- https://raw.githubusercontent.com/facebook/react-native/v0.83.10/packages/react-native/Libraries/Components/Keyboard/Keyboard.js
- https://raw.githubusercontent.com/facebook/react-native/v0.83.10/packages/react-native/Libraries/Components/Keyboard/Keyboard.d.ts
- https://raw.githubusercontent.com/facebook/react-native/v0.83.10/packages/react-native/Libraries/Components/Keyboard/NativeKeyboardObserver.js
- https://raw.githubusercontent.com/facebook/react-native/v0.83.10/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/ReactRootView.java
- https://raw.githubusercontent.com/facebook/react-native/v0.83.10/packages/react-native/src/private/specs_DEPRECATED/modules/NativeKeyboardObserver.js
- …and 28 more

Script executed:

set -eu
printf '%s\n' '--- keyboard occlusion ---'
git show be4beeb57ced2c8b19489f1127dc664f739ee08a:mobile/src/platform/keyboard-occlusion.ts | nl -ba
printf '%s\n' '--- keyboard occlusion tests ---'
git show be4beeb57ced2c8b19489f1127dc664f739ee08a:mobile/src/platform/keyboard-occlusion.test.tsx | nl -ba | sed -n '1,290p'
printf '%s\n' '--- TextInputModal ---'
git show be4beeb57ced2c8b19489f1127dc664f739ee08a:mobile/src/components/TextInputModal.tsx | nl -ba
printf '%s\n' '--- terminal rename modal ---'
git show be4beeb57ced2c8b19489f1127dc664f739ee08a:mobile/src/session/MobileTerminalRenameModal.tsx | nl -ba
printf '%s\n' '--- relevant rename callers ---'
rg -n -F --glob '*.ts' --glob '*.tsx' -e 'MobileTerminalRenameModal' -e 'setRename' -e 'renameTerminal' mobile/src mobile/app || test "$?" -eq 1

Repository: stablyai/orca

Length of output: 25854


Seed from the latest keyboard frame.

React Native 0.83.10 updates Keyboard.metrics() on keyboardDidShow, not on frame changes. The keyboard seam forwards iOS frame changes but does not retain their height. If the keyboard changes height without another show event before TextInputModal opens, seedKeyboard can use the old height and leave the prompt partly behind the keyboard. Retain the latest visible height in the keyboard seam and seed from it.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Nitpick comments: 1
mobile/src/components/bottom-drawer/mounted-bottom-drawer-keyboard-seam.test.tsx: 1

73-73: Trivial · Functional Correctness · Quick win

Keep the mocked shared value across renders.

Seeding updates React state and rerenders the drawer. The current mock replaces keyboardOffset on that render, while the test still passes because harness.writes retains the earlier 300 write. The empty useAnimatedStyle mock also prevents the test from checking the content-sized sheet’s translateY.

Preserve shared-value identity and assert the resulting transform with the hardcoded seeded inset.

Review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 972cb1d5-a558-42f1-b935-8a6ede1c2072
Commits

Reviewing files that changed from the base of the PR and between be4beeb and d6d57c2.

Files selected for processing: 1
  • mobile/src/components/bottom-drawer/mounted-bottom-drawer-keyboard-seam.test.tsx

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.

Comment on lines +143 to +148
it('docks a fill sheet on a keyboard already up when it opens', () => {
harness.current = 300
const renderer = render(true)
expect(marginBottom(renderer)).toBe(300)
act(() => renderer.unmount())
})

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor · Maintainability & Code Quality · Quick win

Remove the added cases that pass without this change.

Four added cases pass when the production change is reverted:

  • the fill-sheet case (lines 143-148)
  • the default content-sized case (lines 150-158)
  • the event-animation case (lines 167-177)
  • the unsubscribe case (lines 179-184)

AGENTS.md allows an added test only when it fails if the change is reverted. Keep the opt-in case (lines 160-165). It fails on revert and checks the new seedKeyboard behavior.

Two of the removed cases assert animation timing: the default content-sized case (line 156) and the event-animation case (lines 171 and 175). AGENTS.md prohibits timing assertions. Those cases contain three timing assertions in total. The fill-sheet and unsubscribe cases assert no timing.

Delete the helpers that become unused: keyboardShows, keyboardHides, and marginBottom.

Suggested fix
-function keyboardShows(height: number, duration: number): void {
-  act(() => harness.show?.(height, duration))
-}
-
-function keyboardHides(duration: number): void {
-  act(() => harness.hide?.(duration))
-}
-
-function marginBottom(renderer: ReactTestRenderer): unknown {
-  const node = renderer.root.find((candidate) => candidate.props.testID === 'bottom-drawer-sheet')
-  const style: unknown[] = [node.props.style].flat(Infinity)
-  return Object.assign({}, ...style.filter((entry) => typeof entry === 'object' && entry !== null))
-    .marginBottom
-}
-
-  it('docks a fill sheet on a keyboard already up when it opens', () => {
-    harness.current = 300
-    const renderer = render(true)
-    expect(marginBottom(renderer)).toBe(300)
-    act(() => renderer.unmount())
-  })
-
-  it('does not seed a content-sized sheet from a keyboard already up', () => {
-    harness.current = 300
-    const renderer = render(false)
-    expect(harness.writes).not.toContain(300)
-    // It still rides the keyboard's next event.
-    keyboardShows(310, 0)
-    expect(harness.timings).toContainEqual({ to: 310, duration: 250 })
-    act(() => renderer.unmount())
-  })
-
-  it('lifts with the event duration and drops back when the keyboard hides', () => {
-    const renderer = render(true)
-    keyboardShows(280, 120)
-    expect(marginBottom(renderer)).toBe(280)
-    expect(harness.timings).toContainEqual({ to: 280, duration: 120 })
-    keyboardHides(0)
-    expect(marginBottom(renderer)).toBe(0)
-    // An event without a duration still animates.
-    expect(harness.timings).toContainEqual({ to: 0, duration: 250 })
-    act(() => renderer.unmount())
-  })
-
-  it('unsubscribes on unmount', () => {
-    const renderer = render(true)
-    expect(harness.show).not.toBeNull()
-    act(() => renderer.unmount())
-    expect(harness.show).toBeNull()
-  })

@nwparker

nwparker commented Nov 5, 2026

Copy link
Copy Markdown
Contributor

Closing as superseded — the linked issue (#32025) has been resolved on main by #33760 (33a86f3352). Thanks for the contribution!

@nwparker nwparker closed this Nov 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(mobile): Rename Terminal modal is occluded by soft keyboard when double-tapping tab with IME already open

2 participants