feat: Remove pull to refresh indicator #1448
Conversation
Removed indicator in iOS for pull to refresh Added constants and comments Removed unused nav list icon
WalkthroughThe pull request introduces several improvements to the conversation and list components across the application. The changes primarily focus on refactoring import paths, adding platform-specific refresh thresholds, and enhancing scroll and refresh handling mechanisms. The modifications aim to improve the consistency of component imports, optimize list refresh behavior for iOS and Android platforms, and implement more robust refresh and scroll event management. Changes
Suggested Reviewers
Poem
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 (
|
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
components/ConversationFlastList/ConversationFlashList.constants.ts
Outdated
Show resolved
Hide resolved
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 |
![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
🧹 Nitpick comments (3)
components/ConversationFlastList/ConversationFlashList.tsx (3)
72-72: Maintain concurrency with arefreshingRef.
Using a mutable ref to avoid multiple concurrent refresh calls is a valid approach. Consider a friendlier approach to concurrency control (e.g., a state variable or a promise lock utility), but for now, this is acceptable.
99-110: Solid refresh handler with try-catch-finally.
Theconsole.logusage for debugging is fine, but consider providing user feedback in production code (e.g., a toast or user-friendly message) rather than a plain console log.
138-139: Conditional refresh props for Android only.
This is consistent with the new iOS refresh logic triggered by scroll offset. Be sure to unify or document these different approaches for clarity.Also applies to: 143-143
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
components/ConversationFlastList/ConversationFlashList.constants.ts(1 hunks)components/ConversationFlastList/ConversationFlashList.tsx(4 hunks)features/blocked-chats/ConversationBlockedListNav.tsx(1 hunks)features/conversation/conversation-list.contstants.ts(1 hunks)features/conversation/conversation.tsx(3 hunks)screens/ConversationList.tsx(1 hunks)screens/Navigation/ConversationListNav.tsx(0 hunks)screens/Navigation/ConversationRequestsListNav.ios.tsx(1 hunks)screens/Navigation/ConversationRequestsListNav.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- screens/Navigation/ConversationListNav.tsx
✅ Files skipped from review due to trivial changes (5)
- screens/ConversationList.tsx
- features/conversation/conversation-list.contstants.ts
- screens/Navigation/ConversationRequestsListNav.tsx
- components/ConversationFlastList/ConversationFlashList.constants.ts
- features/blocked-chats/ConversationBlockedListNav.tsx
🔇 Additional comments (9)
components/ConversationFlastList/ConversationFlashList.tsx (4)
6-13: Imports for RN event handling look good.
No issues here. The newly imported types fromreact-nativewill facilitate scroll event handling across platforms.
15-29: Clean and descriptive imports.
The additional imports likeHiddenRequestsButtonand constants fromConversationFlashList.constantsalign well with the new refresh logic.
37-37: Asynchronous refetch is a good improvement.
Switchingrefetchto return aPromise<unknown>is necessary to accommodate async logic (e.g.,await refetch(...)).
112-125: Great synergy between scroll events and refresh logic.
The threshold-based check for platform-specific bounces is appropriate. Ensure that there are no conflicts with theFlashListbuilt-in refresh on Android.features/conversation/conversation.tsx (5)
77-82: Helpful new imports for handling scroll-based refresh.
These constants and event types enable a cohesive approach to iOS vs. Android refresh strategies.
195-196:refreshingRefreference is consistent with the design.
Matches the approach inConversationFlashList. Good for controlling concurrent refresh attempts.
234-243: Error handling inhandleRefreshis appropriate.
Thetry-catch-finallypattern ensuresrefreshingRefis always reset, avoiding stuck states.
245-253: Scroll-based refresh logic is well-implemented.
Makes sense to only trigger refresh when scrolled above the threshold. Confirm through testing that it doesn’t conflict with any built-in gestures, especially on iOS.
259-260: ConditionalonRefreshsynchronizes with the new approach.
Only Android uses the built-in refresh, while iOS uses scroll-based detection. This separation is coherent.
| @@ -20,7 +20,7 @@ import { | |||
| } from "./Navigation"; | |||
| import ActivityIndicator from "../../components/ActivityIndicator/ActivityIndicator"; | |||
| import Button from "../../components/Button/Button"; | |||
| import ConversationFlashList from "../../components/ConversationFlashList"; | |||
| import ConversationFlashList from "@components/ConversationFlastList/ConversationFlashList"; | |||
There was a problem hiding this comment.
Fix the import path to avoid the "FlastList" typo.
It looks like the directory name was intended to be "FlashList" instead of "FlastList." This will likely cause an import error if the directory name is spelled incorrectly.
-import ConversationFlashList from "@components/ConversationFlastList/ConversationFlashList";
+import ConversationFlashList from "@components/ConversationFlashList/ConversationFlashList";📝 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.
| import ConversationFlashList from "@components/ConversationFlastList/ConversationFlashList"; | |
| import ConversationFlashList from "@components/ConversationFlashList/ConversationFlashList"; |
| @@ -0,0 +1,6 @@ | |||
| import { Platform } from "react-native"; | |||
|
|
|||
| // iOS has it's own bounce and search bar, so we need to set a different threshold | |||
There was a problem hiding this comment.
There's a typo in the comment - it's should be its since this is the possessive form. The current text reads:
// iOS has it's own bounce and search bar
but should be:
// iOS has its own bounce and search bar
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
components/ConversationFlashList/ConversationFlashList.tsx (1)
72-72: Use a ref for concurrency control.
Using arefreshingRefto avoid parallel refresh calls is a solid approach. Consider encapsulating this state in a React state variable if you need the UI to re-render based on the refresh state (e.g., disabling buttons), but for a purely internal guard, this ref is fine.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
components/ConversationFlashList/ConversationFlashList.constants.ts(1 hunks)components/ConversationFlashList/ConversationFlashList.tsx(4 hunks)features/blocked-chats/ConversationBlockedListNav.tsx(1 hunks)screens/ConversationList.tsx(1 hunks)screens/Navigation/ConversationRequestsListNav.ios.tsx(1 hunks)screens/Navigation/ConversationRequestsListNav.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- components/ConversationFlashList/ConversationFlashList.constants.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- screens/ConversationList.tsx
- screens/Navigation/ConversationRequestsListNav.ios.tsx
- screens/Navigation/ConversationRequestsListNav.tsx
- features/blocked-chats/ConversationBlockedListNav.tsx
🔇 Additional comments (5)
components/ConversationFlashList/ConversationFlashList.tsx (5)
6-13: Imports look correct and appropriately scoped.
These additional imports (NativeScrollEvent,NativeSyntheticEvent, etc.) are well-aligned with the new scroll event handling approach and are needed for the iOS-specific refresh logic.
37-37: Async refetch property is a good enhancement.
Switching to an async signature accommodates more robust data fetching scenarios. Ensure that all call sites are updated accordingly to handle thePromise.
99-110: Robust error handling in the refresh logic.
The try-catch-finally block ensures that the refresh flag is properly reset. This is beneficial for resilience. Consider adding user-facing feedback in the catch block if relevant to indicate a failed refresh.
112-125: Clean iOS-specific refresh trigger.
Tying the refresh logic to scrolling beyondCONVERSATION_FLASH_LIST_REFRESH_THRESHOLDeffectively removes the pull-to-refresh indicator on iOS while retaining an instinctive scroll-based refresh trigger. This addresses the PR objective of removing the iOS pull-to-refresh UI component.
138-139: Platform-conditional refresh props and event handling.
Conditionally applyingonRefreshandrefreshingonly for Android aligns with your objective to remove the pull-to-refresh indicator for iOS. TheonScrollprop uses the new iOS threshold-based refresh mechanism. This distinction is clear and logically organized.Also applies to: 143-143
Removed indicator in iOS for pull to refresh
Added constants and comments
Removed unused nav list icon
Summary by CodeRabbit
Based on the comprehensive summary, here are the release notes:
New Features
Refactor
Chores
These changes improve the user experience by optimizing list interactions and refreshing mechanisms across iOS and Android platforms.