Fix message reactions overlay gesture handling#1424
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughPrevent taps/long-presses on previewed messages from opening the fullscreen gallery and add tap-to-dismiss regions for the reactions overlay; image/video thumbnail loading was refactored and test hooks for sync image lookup were introduced for snapshot tests. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Overlay as ReactionsOverlayView
participant Message as MessageItemView / MessageContainerView
participant Attachment as Attachment / FullscreenGallery
participant ImageLoader as NukeImageLoader / StreamAsyncImage
User->>Overlay: tap empty overlay area
Note right of Overlay: contentShape + onTapGesture
Overlay-->>User: dismiss overlay
User->>Message: tap message bubble (shownAsPreview)
Note right of Message: highPriority TapGesture swallows tap
Message--x Attachment: tap does not propagate to gallery
User->>Message: long-press on message with attachment
Note right of Message: MessageActionsGestureModifier active only when not preview
alt shownAsPreview == true
Message--x Attachment: long-press ignored for gallery
else
Message->>Attachment: trigger actions / open gallery
end
Note over Message,ImageLoader: Image/video thumbnail loading via StreamAsyncImage / testSyncLookup hook
ImageLoader->>Message: provide thumbnail phase (success/loading/error)
Message-->>User: render thumbnail / overlays
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
…ering attachments
Replace the environment-based coordination with a single-modifier change: switching the message long-press from `simultaneousGesture` to `highPriorityGesture` cancels pending attachment tap recognizers when the long-press is recognized, preventing the gallery from opening on finger release.
nuno-vieira
force-pushed
the
fix/message-reactions-overlay-gestures
branch
from
830e4fd to
8bef27f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Sources/StreamChatSwiftUI/ChatMessageList/Reactions/ReactionsOverlayView.swift (1)
86-91: Dismiss-on-empty-space fix looks correct.Adding
.contentShape(Rectangle()) + .onTapGestureto theColor.clearinside theGeometryReaderis what actually makes the empty area dismiss the overlay —Color.clearalready participates in hit-testing in SwiftUI, which is why taps there were previously being swallowed silently.One small note: since this
Color.clearfills the entireGeometryReaderand sits abovebackgroundViewin the outerZStack, the pre-existing.onTapGestureonbackgroundView(around line 196) is effectively shadowed for most of the screen. Not a functional issue, just potentially dead-ish code you could prune in a follow-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/StreamChatSwiftUI/ChatMessageList/Reactions/ReactionsOverlayView.swift` around lines 86 - 91, The empty-area-dismiss behavior was added by making the Color.clear inside the GeometryReader participate in hit-testing via .contentShape(Rectangle()) and attach an .onTapGesture that calls dismissReactionsOverlay; keep this change but remove or consider pruning the now-shadowed .onTapGesture on backgroundView (inside the outer ZStack) since the Color.clear covers most of the screen and renders that handler dead code—update or delete the backgroundView's tap handler to avoid redundancy and ensure dismissReactionsOverlay remains the single tap-to-dismiss entrypoint.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@Sources/StreamChatSwiftUI/ChatMessageList/Reactions/ReactionsOverlayView.swift`:
- Around line 86-91: The empty-area-dismiss behavior was added by making the
Color.clear inside the GeometryReader participate in hit-testing via
.contentShape(Rectangle()) and attach an .onTapGesture that calls
dismissReactionsOverlay; keep this change but remove or consider pruning the
now-shadowed .onTapGesture on backgroundView (inside the outer ZStack) since the
Color.clear covers most of the screen and renders that handler dead code—update
or delete the backgroundView's tap handler to avoid redundancy and ensure
dismissReactionsOverlay remains the single tap-to-dismiss entrypoint.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a969694f-2a87-4b96-9749-c114ce25856f
📒 Files selected for processing (21)
CHANGELOG.mdDemoAppSwiftUI/ChannelHeader/NewChatViewModel.swiftDemoAppSwiftUI/CreateGroupViewModel.swiftSources/StreamChatSwiftUI/ChatMessageList/Gallery/MediaViewer.swiftSources/StreamChatSwiftUI/ChatMessageList/GiphyAttachmentView.swiftSources/StreamChatSwiftUI/ChatMessageList/LinkAttachmentView.swiftSources/StreamChatSwiftUI/ChatMessageList/MediaAttachment.swiftSources/StreamChatSwiftUI/ChatMessageList/MessageContainerView.swiftSources/StreamChatSwiftUI/ChatMessageList/MessageItemView.swiftSources/StreamChatSwiftUI/ChatMessageList/Reactions/ReactionsOverlayView.swiftSources/StreamChatSwiftUI/CommonViews/StreamAsyncImage.swiftSources/StreamChatSwiftUI/Utils.swiftSources/StreamChatSwiftUI/Utils/StreamImageDownloader.swiftStreamChatSwiftUITests/Infrastructure/Mocks/MediaLoader_Mock.swiftStreamChatSwiftUITests/Tests/ChatChannel/AttachmentCommandsPickerView_Tests.swiftStreamChatSwiftUITests/Tests/ChatChannel/MessageComposerView_Tests.swiftStreamChatSwiftUITests/Tests/ChatChannel/MessageView_Tests.swiftStreamChatSwiftUITests/Tests/ChatChannel/PollResultsView_Tests.swiftStreamChatSwiftUITests/Tests/StreamChatTestCase.swiftStreamChatSwiftUITests/Tests/Utils/MediaLoader_Tests.swiftStreamChatSwiftUITests/Tests/Utils/StreamChat_Utils_Tests.swift
💤 Files with no reviewable changes (16)
- StreamChatSwiftUITests/Tests/ChatChannel/AttachmentCommandsPickerView_Tests.swift
- Sources/StreamChatSwiftUI/ChatMessageList/MediaAttachment.swift
- Sources/StreamChatSwiftUI/ChatMessageList/Gallery/MediaViewer.swift
- Sources/StreamChatSwiftUI/ChatMessageList/LinkAttachmentView.swift
- StreamChatSwiftUITests/Tests/Utils/MediaLoader_Tests.swift
- StreamChatSwiftUITests/Infrastructure/Mocks/MediaLoader_Mock.swift
- DemoAppSwiftUI/ChannelHeader/NewChatViewModel.swift
- Sources/StreamChatSwiftUI/CommonViews/StreamAsyncImage.swift
- DemoAppSwiftUI/CreateGroupViewModel.swift
- StreamChatSwiftUITests/Tests/ChatChannel/PollResultsView_Tests.swift
- Sources/StreamChatSwiftUI/ChatMessageList/GiphyAttachmentView.swift
- StreamChatSwiftUITests/Tests/ChatChannel/MessageView_Tests.swift
- Sources/StreamChatSwiftUI/Utils.swift
- StreamChatSwiftUITests/Tests/StreamChatTestCase.swift
- StreamChatSwiftUITests/Tests/ChatChannel/MessageComposerView_Tests.swift
- StreamChatSwiftUITests/Tests/Utils/StreamChat_Utils_Tests.swift
martinmitrevski
left a comment
martinmitrevski
left a comment
There was a problem hiding this comment.
When you tap in the message row to dismiss the overlay, there's a little delay, compared to when you tap on the area above or below to dismiss it.
nuno-vieira
force-pushed
the
fix/message-reactions-overlay-gestures
branch
from
922be7e to
8bef27f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Around line 12-15: Under the "Upcoming" section consolidate the two separate
"### 🐞 Fixed" subsections into a single "### 🐞 Fixed" subsection: move all
bullet entries (e.g., the three fix bullets shown) under one "### 🐞 Fixed"
heading and remove the duplicate heading so the changelog follows the Keep a
Changelog format (Added, Fixed, Changed) with a single Fixed subsection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Fixed ✅ |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Sources/StreamChatSwiftUI/ChatMessageList/MessageItemView.swift (1)
214-232: Minor: branching@ViewBuilderchanges view identity based onshownAsPreview.Because the two branches return structurally different view trees (one with gestures attached, one without), SwiftUI will treat them as different view identities if
shownAsPreviewever toggled at runtime, discarding gesture/animation state. It's initialized once and effectively constant for a givenMessageItemView, so this is safe today — just noting it as a latent pitfall ifshownAsPreviewever becomes reactive. An equivalent and slightly safer pattern would be to always attach the gestures withincluding:gated onshownAsPreview, similar to whatMessageContainerViewdoes for its tap swallow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/StreamChatSwiftUI/ChatMessageList/MessageItemView.swift` around lines 214 - 232, The current `@ViewBuilder` body(content:) in MessageItemView returns different view trees when shownAsPreview toggles (one branch without gestures and one with gestures), which can change view identity; instead always attach the gestures to the content and gate their actions on shownAsPreview so the view structure remains constant. Concretely, keep the onTapGesture and highPriorityGesture(LongPressGesture(minimumDuration: longPressMinimumDuration)) on the content unconditionally, but inside their closures first check shownAsPreview (and isDoubleTapEnabled for the double-tap) and return early if preview mode is active; still call onActionsTriggered() only when allowed. This change should be made in the body(content:) function of MessageItemView (affecting the onTapGesture, isDoubleTapEnabled, longPressMinimumDuration, LongPressGesture and onActionsTriggered references).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Sources/StreamChatSwiftUI/ChatMessageList/MessageItemView.swift`:
- Around line 214-232: The current `@ViewBuilder` body(content:) in
MessageItemView returns different view trees when shownAsPreview toggles (one
branch without gestures and one with gestures), which can change view identity;
instead always attach the gestures to the content and gate their actions on
shownAsPreview so the view structure remains constant. Concretely, keep the
onTapGesture and highPriorityGesture(LongPressGesture(minimumDuration:
longPressMinimumDuration)) on the content unconditionally, but inside their
closures first check shownAsPreview (and isDoubleTapEnabled for the double-tap)
and return early if preview mode is active; still call onActionsTriggered() only
when allowed. This change should be made in the body(content:) function of
MessageItemView (affecting the onTapGesture, isDoubleTapEnabled,
longPressMinimumDuration, LongPressGesture and onActionsTriggered references).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: acaab135-2a11-4999-a338-130cf9cb240c
📒 Files selected for processing (2)
CHANGELOG.mdSources/StreamChatSwiftUI/ChatMessageList/MessageItemView.swift
There was a problem hiding this comment.
🧹 Nitpick comments (2)
Sources/StreamChatSwiftUI/CommonViews/NukeImageLoader.swift (1)
27-55: Test hook implementation looks good.The early-return in
cachedResultand thenonisolated(unsafe)storage are appropriate for a test-only synchronous resolver that defaults tonilin production. One small organizational nit: the// MARK: - Test Hooksection is interleaved betweencachedResultandstoreCachingKey, which slightly fragments the public cache-lookup API. Moving the hook declaration to the end of the type (near the// MARK: - Privatesection) would keep the public lookup/store pair together. Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/StreamChatSwiftUI/CommonViews/NukeImageLoader.swift` around lines 27 - 55, Move the test-only hook declaration nonisolated(unsafe) static var testSyncLookup out of the middle of the public cache API so cachedResult and storeCachingKey remain adjacent; specifically, remove the // MARK: - Test Hook block from between cachedResult(...) and storeCachingKey(...) and reinsert the testSyncLookup declaration at the end of the type near the // MARK: - Private section to keep the public lookup/store pair together.Sources/StreamChatSwiftUI/ChatMessageList/MessageMediaAttachmentContentView.swift (1)
118-128: Use.task(id:)for video thumbnail loading instead ofonAppearguard.The current
onAppear { guard videoPreview == nil else { return } … }pattern has two issues:
Stale thumbnail on source change: If the view position is reused with a different
source.url(which can happen when the items array is reshaped),videoPreviewretains the old attachment's cached image and loading is skipped.Retry re-triggering: After a failed load,
videoPreviewErroris set butvideoPreviewremainsnil. Each re-appearance firesonAppearagain, re-invokingloadVideoThumbnail()and potentially racing with a user-initiated retry tap.Switching to
.task(id: source.url)ties the load to the URL, re-runs on URL changes, and auto-cancels on view disappear. To support manual retry, add avideoRetryTokenand include it in the task id, then have the retry overlay bump the token instead of callingloadVideoThumbnail()directly—mirroring theimageRetryTokenpattern already in use.♻️ Sketch
+ `@State` private var videoRetryToken = 0 + `@ViewBuilder` private var videoThumbnail: some View { ZStack { videoBackground videoOverlays } - .onAppear { - guard videoPreview == nil else { return } - loadVideoThumbnail() - } + .task(id: VideoLoadID(url: source.url, retry: videoRetryToken)) { + videoPreview = nil + loadVideoThumbnail() + } }…and have the retry button in
videoOverlaysbumpvideoRetryToken &+= 1instead of callingloadVideoThumbnail()directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/StreamChatSwiftUI/ChatMessageList/MessageMediaAttachmentContentView.swift` around lines 118 - 128, Replace the onAppear guard-based loading in the videoThumbnail View with a .task(id: ...) driven task that depends on the attachment's source.url and a videoRetryToken so thumbnail loading re-runs when the URL changes or a manual retry is triggered; specifically, remove the onAppear { guard videoPreview == nil ... loadVideoThumbnail() } pattern in videoThumbnail and instead invoke loadVideoThumbnail() inside .task(id: (source.url, videoRetryToken)), add a videoRetryToken Int state (mirroring imageRetryToken) and change the retry button in videoOverlays to bump videoRetryToken (e.g., += 1) rather than calling loadVideoThumbnail() directly, leaving existing videoPreview and videoPreviewError handling intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@Sources/StreamChatSwiftUI/ChatMessageList/MessageMediaAttachmentContentView.swift`:
- Around line 118-128: Replace the onAppear guard-based loading in the
videoThumbnail View with a .task(id: ...) driven task that depends on the
attachment's source.url and a videoRetryToken so thumbnail loading re-runs when
the URL changes or a manual retry is triggered; specifically, remove the
onAppear { guard videoPreview == nil ... loadVideoThumbnail() } pattern in
videoThumbnail and instead invoke loadVideoThumbnail() inside .task(id:
(source.url, videoRetryToken)), add a videoRetryToken Int state (mirroring
imageRetryToken) and change the retry button in videoOverlays to bump
videoRetryToken (e.g., += 1) rather than calling loadVideoThumbnail() directly,
leaving existing videoPreview and videoPreviewError handling intact.
In `@Sources/StreamChatSwiftUI/CommonViews/NukeImageLoader.swift`:
- Around line 27-55: Move the test-only hook declaration nonisolated(unsafe)
static var testSyncLookup out of the middle of the public cache API so
cachedResult and storeCachingKey remain adjacent; specifically, remove the //
MARK: - Test Hook block from between cachedResult(...) and storeCachingKey(...)
and reinsert the testSyncLookup declaration at the end of the type near the //
MARK: - Private section to keep the public lookup/store pair together.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c9dcccd1-3cf4-40ab-ac2a-d1785e720945
⛔ Files ignored due to path filters (7)
StreamChatSwiftUITests/Tests/ChatChannel/__Snapshots__/MessageView_Tests/test_linkAttachmentView_customColors_snapshot.1.pngis excluded by!**/*.pngStreamChatSwiftUITests/Tests/ChatChannel/__Snapshots__/MessageView_Tests/test_linkAttachmentView_snapshot.1.pngis excluded by!**/*.pngStreamChatSwiftUITests/Tests/ChatChannel/__Snapshots__/MessageView_Tests/test_messageViewGiphy_snapshot.1.pngis excluded by!**/*.pngStreamChatSwiftUITests/Tests/ChatChannel/__Snapshots__/MessageView_Tests/test_messageViewPendingGiphy_snapshot.default-light.pngis excluded by!**/*.pngStreamChatSwiftUITests/Tests/ChatChannel/__Snapshots__/MessageView_Tests/test_messageViewPendingGiphy_snapshot.extraExtraExtraLarge-light.pngis excluded by!**/*.pngStreamChatSwiftUITests/Tests/ChatChannel/__Snapshots__/MessageView_Tests/test_messageViewPendingGiphy_snapshot.rightToLeftLayout-default.pngis excluded by!**/*.pngStreamChatSwiftUITests/Tests/ChatChannel/__Snapshots__/MessageView_Tests/test_messageViewPendingGiphy_snapshot.small-dark.pngis excluded by!**/*.png
📒 Files selected for processing (5)
Sources/StreamChatSwiftUI/ChatMessageList/MessageMediaAttachmentContentView.swiftSources/StreamChatSwiftUI/CommonViews/NukeImageLoader.swiftStreamChatSwiftUITests/Infrastructure/Mocks/MediaLoader_Mock.swiftStreamChatSwiftUITests/Tests/CommonViews/NukeImageLoader_Tests.swiftStreamChatSwiftUITests/Tests/StreamChatTestCase.swift
Public Interface🚀 No changes affecting the public interface. |
SDK Size
|
martinmitrevski
left a comment
martinmitrevski
left a comment
There was a problem hiding this comment.
Works great now ✅
StreamChatSwiftUI XCSize
|
|
3 participants
https://linear.app/stream/issue/IOS-1641/v5-qa-message-reactions-overlay-gesture-fixes
🎯 Goal
Fix a set of gesture issues in the reactions overlay view that made the overlay behave unexpectedly when interacting with the previewed message:
🛠 Implementation
MessageItemView: use.contentShape(Rectangle())so the entire cell (including empty lateral space) participates in hit-testing, and switch the message long-press from.simultaneousGestureto.highPriorityGesture. The high-priority long-press wins against child tap gestures, so media attachments no longer fire their tap action when the user is long-pressing.MessageContainerView: when shown as preview inside the overlay, attach a.highPriorityGesture(TapGesture())that swallows taps on the bubble only. This keeps the bubble non-interactive inside the overlay while leaving scrolling and other gestures intact.ReactionsOverlayView: add an.onTapGestureon the message item view wrapper that dismisses the overlay when the user taps the empty cell area around the preview bubble.Net effect:
🎨 Showcase
N/A — behavioral fix, no UI changes.
🧪 Manual Testing Notes
☑️ Contributor Checklist
Summary by CodeRabbit
Bug Fixes
New Features