fix: Swipable too sensitive #1280
Conversation
Added check for translation to handle read/unread setting
WalkthroughThe changes in this pull request focus on enhancing the swipe gesture handling for the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| toggleReadStatus(); | ||
| }, [toggleReadStatus]); | ||
| const translation = ref.current?.state.rowTranslation; | ||
| if (translation && (translation as any)._value > 100) { |
There was a problem hiding this comment.
Would love to not do this, but moving stuff over from V2
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
components/V3DMListItem.tsx (1)
155-159: Improve type safety and threshold configurationWhile the translation threshold check effectively addresses the sensitivity issue, there are a couple of improvements to consider:
- The type assertion
as anycould be replaced with proper typing- The threshold value (100) should be defined as a named constant for better maintainability
Consider applying these improvements:
+const SWIPE_TRANSLATION_THRESHOLD = 100; // pixels + const onLeftSwipe = useCallback(() => { const translation = ref.current?.state.rowTranslation; - if (translation && (translation as any)._value > 100) { + if (translation?._value > SWIPE_TRANSLATION_THRESHOLD) { toggleReadStatus(); } }, [toggleReadStatus, ref]);components/V3GroupConversationListItem.tsx (1)
298-303: Document swipe behavior and thresholdsWhile the implementation effectively addresses the sensitivity issue, it would benefit from documentation explaining:
- The expected swipe behavior
- The reasoning behind the chosen threshold value
- The relationship between swipe distance and read status toggle
Consider adding a JSDoc comment:
+ /** + * Handles left swipe gesture to toggle read status + * Only triggers when swipe distance exceeds threshold to prevent accidental toggles + * @see https://github.com/ephemeraHQ/converse-app/pull/1280 + */ const onLeftSwipe = useCallback(() => { const translation = ref.current?.state.rowTranslation; if (translation && (translation as any)._value > 100) { toggleReadStatus(); } }, [ref, toggleReadStatus]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
components/V3DMListItem.tsx(1 hunks)components/V3GroupConversationListItem.tsx(1 hunks)
🔇 Additional comments (3)
components/V3DMListItem.tsx (2)
155-159: LGTM: Swipe sensitivity fix is well implemented
The addition of a translation threshold check is a good solution to prevent accidental triggering of the read/unread toggle. The implementation:
- Properly checks the current swipe translation
- Only triggers the action when the swipe exceeds a significant distance
- Maintains the existing haptic feedback for user interaction
- Correctly includes the ref in the dependency array
155-159: Verify consistent swipe threshold across components
Since similar changes were made to V3GroupConversationListItem.tsx, we should ensure both components use the same translation threshold for a consistent user experience.
✅ Verification successful
Swipe threshold is consistent across components
The verification shows that both V3DMListItem.tsx and V3GroupConversationListItem.tsx use the same translation threshold value of 100 for triggering the read status toggle, ensuring a consistent swipe behavior across the application.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for translation threshold checks in conversation list items
rg -A 3 "rowTranslation.*_value.*>" --type typescript
Length of output: 90
Script:
#!/bin/bash
# Search for translation threshold checks in conversation list items using ripgrep without file type restriction
rg -A 3 "rowTranslation.*_value.*>"
Length of output: 37
Script:
#!/bin/bash
# First, let's find all relevant list item components
fd "V3.*ListItem.tsx"
# Then search for any translation or swipe related code
rg -A 5 "translation|swipe|rowTranslation" $(fd "V3.*ListItem.tsx")
Length of output: 1199
components/V3GroupConversationListItem.tsx (1)
299-303: Verify consistent swipe threshold across components
Since this PR addresses swipe sensitivity, we should ensure consistent behavior across all swipeable components.
✅ Verification successful
Let me gather more specific information about the swipe threshold implementation in these components.
Swipe threshold implementation is consistent in list items but differs in message replies
The verification reveals:
- Both
V3GroupConversationListItemandV3DMListItemconsistently use the same threshold of 100 for swipe actions to toggle read status RepliableMessageWrapperuses a different threshold variablexTresholdToReplyfor reply actions, which is appropriate as it serves a different purposeConversationListItemdoesn't show any translation threshold, suggesting it might be using a different mechanism
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other swipeable components and their threshold implementations
echo "Searching for swipeable components and their threshold implementations..."
# Look for similar translation checks in other files
rg -A 3 "rowTranslation" --type typescript
# Look for other swipeable components
ast-grep --pattern 'import { Swipeable } from "react-native-gesture-handler"'
Length of output: 736
Script:
#!/bin/bash
# Search for translation threshold checks in the identified components
echo "Checking translation threshold implementations..."
# Look for translation threshold checks
rg -A 5 "translation.*_value" components/V3GroupConversationListItem.tsx components/V3DMListItem.tsx components/ConversationListItem/ConversationListItem.tsx components/Chat/Message/RepliableMessageWrapper.tsx
# Look for any other numeric threshold checks related to swipe
rg -A 5 "translation.*\d+" components/V3GroupConversationListItem.tsx components/V3DMListItem.tsx components/ConversationListItem/ConversationListItem.tsx components/Chat/Message/RepliableMessageWrapper.tsx
Length of output: 2645
| const translation = ref.current?.state.rowTranslation; | ||
| if (translation && (translation as any)._value > 100) { | ||
| toggleReadStatus(); | ||
| } | ||
| }, [ref, toggleReadStatus]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety and maintainability of the swipe threshold check
The current implementation has several issues:
- Using
as anybypasses TypeScript's type checking - Hardcoded threshold value lacks context
- Accessing internal
_valueproperty is fragile
Consider this safer implementation:
- const translation = ref.current?.state.rowTranslation;
- if (translation && (translation as any)._value > 100) {
+ const SWIPE_THRESHOLD = 100; // pixels
+ const translation = ref.current?.state.rowTranslation?.getValue?.();
+ if (typeof translation === 'number' && translation > SWIPE_THRESHOLD) {
toggleReadStatus();
}Also, consider adding these improvements:
- Define the threshold as a constant at component/module level
- Add a comment explaining the threshold's significance
- Consider making the threshold configurable via props if needed by other components
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const translation = ref.current?.state.rowTranslation; | |
| if (translation && (translation as any)._value > 100) { | |
| toggleReadStatus(); | |
| } | |
| }, [ref, toggleReadStatus]); | |
| const SWIPE_THRESHOLD = 100; // pixels | |
| const translation = ref.current?.state.rowTranslation?.getValue?.(); | |
| if (typeof translation === 'number' && translation > SWIPE_THRESHOLD) { | |
| toggleReadStatus(); | |
| } | |
| }, [ref, toggleReadStatus]); |
Performance Comparison ReportSignificant Changes To DurationThere are no entries Meaningless Changes To DurationShow entries
Show details
Render Count ChangesThere are no entries Render IssuesThere are no entries Added ScenariosThere are no entries Removed ScenariosThere are no entries |
Added check for translation to handle read/unread setting
Added check for translation to handle read/unread setting
Closes #1281
Summary by CodeRabbit
New Features
Bug Fixes
Documentation