Skip to content

fix: move multi-select indicators into a leading gutter#1488

Open
agent-p1p wants to merge 1 commit into
masterfrom
pip/whitenoise-android-1485
Open

fix: move multi-select indicators into a leading gutter#1488
agent-p1p wants to merge 1 commit into
masterfrom
pip/whitenoise-android-1485

Conversation

@agent-p1p

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

Copy link
Copy Markdown
Contributor

Fixes #1485

Summary

  • reserve a fixed 40dp leading gutter in selection mode for received and outgoing messages
  • show outlined and checked indicators for unselected and selected selectable rows while preserving the slot for non-selectable system rows
  • center the indicator against tall message rows and subtract the gutter from the bubble width budget
  • tint the full selected row behind its content with stronger AMOLED-aware colors while keeping a transparent full-row input target above nested media/avatar clickables
  • reuse the chat-list selection icon mapping so both surfaces share the same visual language
  • preserve partial-text-selection gesture, semantic, coordinate, and action-menu guards from feat: add partial message text selection #1435 after rebasing onto current master

Verification

  • ./gradlew :app:compileDevZapstoreDebugKotlin :app:compileDevPlayDebugKotlin --no-daemon --stacktrace
  • ./gradlew :app:testDevZapstoreDebugUnitTest :app:testDevPlayDebugUnitTest --no-daemon --stacktrace
  • ./gradlew :app:verifyRoborazziDevZapstoreDebug :app:verifyRoborazziDevPlayDebug --no-daemon --stacktrace
  • ./gradlew :app:ktlintCheck --no-daemon --stacktrace
  • ./gradlew :app:lintDevZapstoreDebug :app:lintDevPlayDebug --no-daemon --stacktrace

All passed locally at c6e7b522. Light, Dark, and AMOLED selection-state Roborazzi baselines were also inspected visually.

Sensitive paths

None.


Open in Stage

@coderabbitai

coderabbitai Bot commented Jul 16, 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: 47 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: 98eef6de-cf8f-4cf4-8e41-8e8378e8d719

📥 Commits

Reviewing files that changed from the base of the PR and between 3f3feb3 and c6e7b52.

⛔ Files ignored due to path filters (3)
  • app/src/test/snapshots/message_bubble_selection_selected_amoled.png is excluded by !**/*.png
  • app/src/test/snapshots/message_bubble_selection_selected_dark.png is excluded by !**/*.png
  • app/src/test/snapshots/message_bubble_selection_selected_light.png is excluded by !**/*.png
📒 Files selected for processing (9)
  • app/src/main/java/dev/ipf/whitenoise/android/ui/chats/ChatRow.kt
  • app/src/main/java/dev/ipf/whitenoise/android/ui/common/SelectionRowIcon.kt
  • app/src/main/java/dev/ipf/whitenoise/android/ui/conversation/messages/MessageBubble.kt
  • app/src/main/java/dev/ipf/whitenoise/android/ui/conversation/messages/MessageBubbleSelection.kt
  • app/src/test/java/dev/ipf/whitenoise/android/ui/conversation/MessageMultiSelectCoverageTest.kt
  • app/src/test/java/dev/ipf/whitenoise/android/ui/conversation/messages/MessageBubbleAmoledStyleTest.kt
  • app/src/test/java/dev/ipf/whitenoise/android/ui/conversation/messages/MessageBubbleSelectionIndicatorCoverageTest.kt
  • app/src/test/java/dev/ipf/whitenoise/android/ui/conversation/messages/MessageBubbleTextTest.kt
  • app/src/test/java/dev/ipf/whitenoise/android/ui/screenshot/MessageBubbleSelectionScreenshotTest.kt
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch pip/whitenoise-android-1485

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.

@stage-review

stage-review Bot commented Jul 16, 2026

Copy link
Copy Markdown

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

Title
1 Extract shared selection icon utility
2 Implement message bubble selection components
3 Integrate leading gutter into MessageBubble
4 Verify selection UI with tests
5 Other changes
Open in Stage

Chapters generated by Stage for commit c6e7b52 on Jul 16, 2026 3:22pm UTC.

@github-actions

github-actions Bot commented Jul 16, 2026

Copy link
Copy Markdown

Preview APK for PR #1488

Home-screen label: 1488 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

BLOCKING:

  • The live PR is now unmergeable with master. PR state is DIRTY after feat: add partial message text selection #1435 (feat: add partial message text selection) merged as 3f3feb32. A three-way merge conflicts in MessageBubble.kt exactly where this PR rewrites row interaction and in MessageMultiSelectCoverageTest.kt. Rebase onto current master and resolve by preserving feat: add partial message text selection #1435's text-selection invariants: textSelectionMode must remain in the reply-swipe, raw long-press, semantic long-click, and action-menu guards; the row bounds must continue recording both left/top coordinates; and the longPressWindowPosition state/reset behavior must survive. Then combine the new gutter assertions with feat: add partial message text selection #1435's updated multi-select expectations and rerun both flavors. The currently green checks were produced against the pre-feat: add partial message text selection #1435 base and do not verify the required resolution.

SUGGESTIONS:

  • MessageBubbleSelection.kt:75-89 gives the gutter only a width. Its height therefore wraps the 24dp icon, and the parent Row in MessageBubble.kt:494 has the default top alignment. On multiline/media messages the circle sits at the top of the row rather than centered alongside the message. Give the gutter a parent-height-aware vertical alignment and add a tall-row position assertion.
  • The requested own/received regression coverage is not present. MessageBubbleSelectionIndicatorCoverageTest tests the icon mapping and a simple start-aligned Row, but never exercises the mine branch/weighted spacer at MessageBubble.kt:603-613, never verifies the selected icon actually rendered, and does not behaviorally check long-message bounds or LazyColumn scroll anchoring. Add direction-sensitive layout coverage at minimum; the current source-string assertions would miss an outgoing placement regression.
  • The tint tests lock alpha constants rather than verifying the rendered result. With the repository palette, compositing the proposed tint over the actual row surface yields approximately 1.16:1 in Light, 1.34:1 in Dark, and 1.46:1 in AMOLED. That may still read as the faint wash Multi-select: checkmark overlays the bubble and unselected messages show no indicator — move it to a leading gutter #1485 is trying to replace, especially because opaque bubble surfaces cover the row tint. Add a selection-state Roborazzi fixture (Light/Dark/AMOLED) or otherwise verify the tint is obvious at a glance before keeping these values.

NITPICK:
none

FOLLOW-UP:

SUMMARY: The leading-gutter structure is directionally correct and the focused dual-flavor tests, compilation, and ktlint pass at 8d5fac23. The PR must first be rebased over #1435 without dropping its partial-text-selection interaction guards. The visual alignment and acceptance-test gaps should be addressed during that resolution.

VERDICT: REQUEST_CHANGES
SENSITIVE_PATHS: none

Verification at exact head 8d5fac239df2e2cbfdcef7bd2f710b0525610848:

  • Focused MessageBubbleSelectionIndicatorCoverageTest, MessageBubbleAmoledStyleTest, and MessageMultiSelectCoverageTest, both Dev Zapstore and Dev Play: passed
  • :app:compileDevZapstoreDebugKotlin, :app:compileDevPlayDebugKotlin, :app:ktlintCheck: passed
  • Live GitHub checks: all reported jobs green, but the PR became DIRTY when feat: add partial message text selection #1435 merged after those checks

@agent-p1p
agent-p1p force-pushed the pip/whitenoise-android-1485 branch from 8d5fac2 to c6e7b52 Compare July 16, 2026 15:22
@agent-p1p

Copy link
Copy Markdown
Contributor Author

Addressed the blocking review at c6e7b52: rebased onto current master while preserving #1435 text-selection guards and coordinates; centered the leading indicator for tall rows; added incoming/outgoing layout coverage; and added visually inspected Light/Dark/AMOLED Roborazzi baselines. Exact local CI commands all pass. @coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 16, 2026

Copy link
Copy Markdown
🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '== PR metadata =='
gh pr view 1488 --repo marmot-protocol/whitenoise-android \
  --json number,headRefOid,baseRefOid,mergeStateStatus,isDraft,changedFiles \
  --jq '{number, headRefOid, baseRefOid, mergeStateStatus, isDraft, changedFiles}'

echo
echo '== Diff summary against merge-base =='
base="$(git merge-base HEAD origin/master)"
printf 'merge-base: %s\n' "$base"
git diff --stat "$base"...HEAD
git diff --name-only "$base"...HEAD

echo
echo '== Candidate source/test file locations =='
fd -a -t f 'MessageBubble(\.kt|Selection\.kt|.*CoverageTest\.kt|.*Amoled.*Test\.kt)$' . | sort

echo
echo '== Kotlin structural outlines for changed Kotlin files =='
git diff --name-only "$base"...HEAD -- '*.kt' | while IFS= read -r file; do
  echo "--- $file ---"
  ast-grep outline "$file" --items all --view compact || true
done

Length of output: 6155


🏁 Script executed:

#!/bin/bash
set -euo pipefail

base="$(git merge-base HEAD origin/master)"

echo '== AST outlines =='
for file in \
  app/src/main/java/dev/ipf/whitenoise/android/ui/conversation/messages/MessageBubble.kt \
  app/src/main/java/dev/ipf/whitenoise/android/ui/conversation/messages/MessageBubbleSelection.kt \
  app/src/main/java/dev/ipf/whitenoise/android/ui/common/SelectionRowIcon.kt \
  app/src/test/java/dev/ipf/whitenoise/android/ui/conversation/MessageMultiSelectCoverageTest.kt \
  app/src/test/java/dev/ipf/whitenoise/android/ui/conversation/messages/MessageBubbleSelectionIndicatorCoverageTest.kt \
  app/src/test/java/dev/ipf/whitenoise/android/ui/screenshot/MessageBubbleSelectionScreenshotTest.kt; do
  echo "--- $file ---"
  ast-grep outline "$file" --items all --view signatures || true
done

echo
echo '== Production Kotlin diff =='
git diff --unified=80 "$base"...HEAD -- \
  app/src/main/java/dev/ipf/whitenoise/android/ui/conversation/messages/MessageBubble.kt \
  app/src/main/java/dev/ipf/whitenoise/android/ui/conversation/messages/MessageBubbleSelection.kt \
  app/src/main/java/dev/ipf/whitenoise/android/ui/common/SelectionRowIcon.kt \
  app/src/main/java/dev/ipf/whitenoise/android/ui/chats/ChatRow.kt

echo
echo '== Test Kotlin diff =='
git diff --unified=50 "$base"...HEAD -- \
  app/src/test/java/dev/ipf/whitenoise/android/ui/conversation/MessageMultiSelectCoverageTest.kt \
  app/src/test/java/dev/ipf/whitenoise/android/ui/conversation/messages/MessageBubbleAmoledStyleTest.kt \
  app/src/test/java/dev/ipf/whitenoise/android/ui/conversation/messages/MessageBubbleSelectionIndicatorCoverageTest.kt \
  app/src/test/java/dev/ipf/whitenoise/android/ui/conversation/messages/MessageBubbleTextTest.kt \
  app/src/test/java/dev/ipf/whitenoise/android/ui/screenshot/MessageBubbleSelectionScreenshotTest.kt

echo
echo '== Guard and coordinate contexts in MessageBubble.kt =='
rg -n -C 8 \
  'textSelectionMode|longPressWindowPosition|positionInWindow|onLongClick|combinedClickable|detect.*Gesture|onGloballyPositioned|onClickLabel' \
  app/src/main/java/dev/ipf/whitenoise/android/ui/conversation/messages/MessageBubble.kt

Length of output: 126392


🏁 Script executed:

#!/bin/bash
set -euo pipefail

file='app/src/main/java/dev/ipf/whitenoise/android/ui/conversation/messages/MessageBubble.kt'

echo '== Long-press/action-menu state assignments and close paths =='
rg -n -C 5 \
  '(longPressWindowPosition|longPressWindowY|onActionMenuOpenChange)\s*(=|\()' \
  "$file"

echo
echo '== `#1435` base interaction code for comparison =='
git show 3f3feb32beea466649f3581f11fd47df2ceecc1c:"$file" | \
  sed -n '560,710p;925,970p;1475,1510p;1595,1635p'

echo
echo '== Complete focused test sources =='
sed -n '1,380p' \
  app/src/test/java/dev/ipf/whitenoise/android/ui/conversation/messages/MessageBubbleSelectionIndicatorCoverageTest.kt
sed -n '1,180p' \
  app/src/test/java/dev/ipf/whitenoise/android/ui/screenshot/MessageBubbleSelectionScreenshotTest.kt

echo
echo '== GitHub mergeability and checks =='
gh pr view 1488 --repo marmot-protocol/whitenoise-android \
  --json mergeStateStatus,reviewDecision,statusCheckRollup \
  --jq '{mergeStateStatus, reviewDecision, checks: [.statusCheckRollup[] | {name, status, conclusion}]}'

Length of output: 42634


@agent-p1p Follow-up review found no new blocking issues.

Confirmed at c6e7b522:

  • The PR is rebased on #1435 (3f3feb32).
  • Partial-text-selection guards, row left/top coordinate recording, and long-press state behavior are preserved.
  • The leading gutter is centered for tall rows, remains leading for incoming and outgoing messages, and reduces the bubble width budget.
  • Coverage now includes direction-sensitive layout behavior and Light/Dark/AMOLED selection screenshots.

The PR is still a draft and GitHub’s compile/test/lint and APK checks are currently in progress, so mergeStateStatus remains UNSTABLE.


If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@agent-p1p
agent-p1p marked this pull request as ready for review July 16, 2026 15:38
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.

Multi-select: checkmark overlays the bubble and unselected messages show no indicator — move it to a leading gutter

1 participant