Skip to content

refactor: share keyboard-safe popup behavior#1418

Merged
mubarakcoded merged 2 commits into
masterfrom
pip/whitenoise-android-1413
Jul 15, 2026
Merged

refactor: share keyboard-safe popup behavior#1418
mubarakcoded merged 2 commits into
masterfrom
pip/whitenoise-android-1413

Conversation

@agent-p1p

@agent-p1p agent-p1p commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Fixes #1413

Summary

  • Extracts the non-focusable Popup + tap-consuming scrim + host BackHandler pattern into KeyboardSafePopup.
  • Migrates MessageActionMenu without changing its touch-positioning or first-frame height logic.
  • Migrates ReactionDetailsSheet to an IME-safe bottom-anchored popup; oversized content remains bottom-pinned to the host visible frame.
  • Leaves ForwardMessageSheet on ModalBottomSheet because its search field intentionally takes focus.
  • Adds regression coverage for focus/dismissal plumbing, caller-owned positioning, IME-shrunk/oversized bottom anchoring, stale measured-height reset, both migrations, and the ForwardSheet exception.

Verification

  • ./gradlew :app:compileDevDebugKotlin --no-daemon --stacktrace
  • ./gradlew :app:testDevDebugUnitTest --no-daemon --stacktrace
  • ./gradlew :app:verifyRoborazziDevDebug --no-daemon --stacktrace
  • ./gradlew :app:ktlintCheck --no-daemon --stacktrace
  • ./gradlew :app:lintDevDebug --no-daemon --stacktrace
  • git diff --cached --check

All passed locally. No connected device tests were run.

Sensitive paths

None.


Open in Stage

Extract the non-focusable popup, scrim, and BackHandler pattern and
migrate the message action menu plus reaction details sheet to it.

Fixes #1413
@stage-review

stage-review Bot commented Jul 15, 2026

Copy link
Copy Markdown

Ready to review this PR? Stage has broken it down into 4 individual chapters for you:

Title
1 Extract shared keyboard-safe popup logic
2 Migrate MessageActionMenu to KeyboardSafePopup
3 Convert ReactionDetailsSheet to non-focusable popup
4 Add regression tests for popup behavior
Open in Stage

Chapters generated by Stage for commit 7c652de on Jul 15, 2026 9:21am UTC.

@coderabbitai

coderabbitai Bot commented Jul 15, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@agent-p1p, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 7 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 428d03e9-5c3d-4c2b-aaea-afcdc90fefe3

📥 Commits

Reviewing files that changed from the base of the PR and between 9550127 and 7c652de.

📒 Files selected for processing (4)
  • app/src/main/java/dev/ipf/whitenoise/android/ui/conversation/messages/MessageActions.kt
  • app/src/main/java/dev/ipf/whitenoise/android/ui/conversation/reactions/Reactions.kt
  • app/src/main/java/dev/ipf/whitenoise/android/ui/design/KeyboardSafePopup.kt
  • app/src/test/java/dev/ipf/whitenoise/android/ui/design/KeyboardSafePopupCoverageTest.kt
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch pip/whitenoise-android-1413

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

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Jul 15, 2026

Copy link
Copy Markdown

Preview APK for PR #1418

Home-screen label: 1418 White Noise with the blueprint launcher icon — installs side-by-side with the production and staging apps.

Install directly — one tap, no GitHub sign-in (public, content-addressed .apk). ARM64 only.

Every push to this PR rebuilds and updates this comment.

@agent-p1p

Copy link
Copy Markdown
Contributor Author

Adversarial review — changes requested

Blocking

  • KeyboardSafePopup.kt:49-56 — do not claim the system Back-gesture edges. Both popup windows use PopupProperties without overriding excludeFromSystemGesture, whose Android default is true. In the resolved Compose UI 1.12.0-alpha03, PopupLayout responds by assigning systemGestureExclusionRects = Rect(0, 0, windowSize.width, windowSize.height). The scrim at lines 93-105 fills that visible window; the bottom-anchored content also reaches the gesture edge. On gesture-navigation devices this can prevent the system/predictive Back gesture from reaching the host-window BackHandler, contradicting this component's Back-dismiss contract. Set excludeFromSystemGesture = false for these keyboard-preserving popups and add a regression guard. PR Fix keyboard dismissal when opening message sheets #1417's KeyboardPreservingBottomSheet already carries this safeguard.

Suggestions

  • KeyboardSafePopupCoverageTest.kt only checks source text plus the pure position-provider arithmetic. It does not exercise the actual three-window interaction (host + scrim popup + content popup), so it cannot catch IME loss, edge-Back suppression, or outside-tap click-through. Please add a Compose/instrumentation regression if the test harness permits; at minimum, statically guard the required excludeFromSystemGesture = false property.
  • Integration: PR Fix keyboard dismissal when opening message sheets #1417 and this PR both replace ReactionDetailsSheet and touch Reactions.kt. Choose the surviving abstraction/merge order and rebase the other PR; merging both migrations as-is will duplicate/conflict rather than leave one keyboard-preserving sheet path.

Validation

  • Exact reviewed head: 5f385972c0b21a66a90266d5bb8d4e1a1c711271
  • Targeted KeyboardSafePopupCoverageTest: pass
  • compileDevDebugKotlin and ktlintCheck with --rerun-tasks: pass
  • lintDevDebug: pass
  • GitHub CI: green

Verdict: 1 blocking finding, 2 suggestions, 0 nitpicks.

Prevent the full-window keyboard-safe popups from claiming Android's system gesture exclusion area, and guard the requirement with a regression test.\n\nFixes #1413
@agent-p1p

Copy link
Copy Markdown
Contributor Author

Addressed the blocking system-gesture finding in signed commit 7c652dee: the shared popup properties now set excludeFromSystemGesture = false, with a red-first regression guard. All Android CI-equivalent checks pass locally.

Integration decision: land #1418 first as the shared abstraction, then rebase #1417 and migrate its remaining sheets onto KeyboardSafePopup. The current duplicate #1417 implementation should not merge as-is after this PR.

@agent-p1p
agent-p1p marked this pull request as ready for review July 15, 2026 09:32
@mubarakcoded
mubarakcoded merged commit f209470 into master Jul 15, 2026
4 checks passed
@mubarakcoded
mubarakcoded deleted the pip/whitenoise-android-1413 branch July 15, 2026 15:59
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.

Keyboard-safe Popup+scrim+BackHandler pattern (#284) never extracted — #1396 plans to hand-copy it a 4th/5th/6th time instead of sharing it

2 participants