Revert "[No QA] Add more context to ActivityIndicator logs"#71614
Revert "[No QA] Add more context to ActivityIndicator logs"#71614
ActivityIndicator logs"#71614Conversation
This comment has been minimized.
This comment has been minimized.
|
🚧 @Julesssss has triggered a test Expensify/App build. You can view the workflow run here. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| clearTimeout(timeoutId); | ||
| }; | ||
| }, [extraLoadingContext, timeout]); | ||
| }, [timeout]); |
There was a problem hiding this comment.
⚠️ PERF-6
The useEffect dependency array includes [timeout] which will cause the effect to re-run and create new timers whenever the timeout value changes. While timeout is typically a constant, this could lead to unnecessary timer creation if the prop changes frequently.
Consider whether timeout is expected to change during the component's lifecycle. If it's expected to be stable, this is fine. If it might change frequently, consider memoizing the timeout value or restructuring to avoid recreation of timers.
| }; | ||
|
|
||
| function FullScreenLoadingIndicator({style, iconSize = CONST.ACTIVITY_INDICATOR_SIZE.LARGE, testID = '', extraLoadingContext}: FullScreenLoadingIndicatorProps) { | ||
| function FullScreenLoadingIndicator({style, iconSize = CONST.ACTIVITY_INDICATOR_SIZE.LARGE, testID = ''}: FullScreenLoadingIndicatorProps) { |
There was a problem hiding this comment.
✅ Good Practice
The default parameter assignment testID = '' is well implemented. This provides a clean fallback value and ensures the testID prop is always defined when passed to the ActivityIndicator component.
| }); | ||
| }, timeout); | ||
|
|
||
| return () => { |
There was a problem hiding this comment.
✅ Good Practice
The cleanup function properly clears the timeout, preventing memory leaks when the component unmounts or when the effect re-runs. This is essential for timer management in React components.
| import Log from '@libs/Log'; | ||
| import CONST from '@src/CONST'; | ||
|
|
||
| type ActivityIndicatorProps = RNActivityIndicatorProps & { |
There was a problem hiding this comment.
✅ Type Safety
Good use of intersection types with RNActivityIndicatorProps & {...} to extend the native ActivityIndicator props while adding custom properties. This ensures type safety and allows all native props to be passed through.
| }, [timeout]); | ||
|
|
||
| return ( | ||
| <RNActivityIndicator |
There was a problem hiding this comment.
💡 Accessibility Consideration
Consider adding accessibility properties for better screen reader support:
<RNActivityIndicator
color={theme.spinner}
accessibilityLabel="Loading"
accessibilityHint="Please wait while content loads"
// eslint-disable-next-line react/jsx-props-no-spreading
{...rest}
/>This would improve the experience for users with accessibility needs, unless these props are intentionally passed through the rest spread.
| function FullScreenLoadingIndicator({style, iconSize = CONST.ACTIVITY_INDICATOR_SIZE.LARGE, testID = ''}: FullScreenLoadingIndicatorProps) { | ||
| const styles = useThemeStyles(); | ||
| return ( | ||
| <View style={[StyleSheet.absoluteFillObject, styles.fullScreenLoading, style]}> |
There was a problem hiding this comment.
✅ Good Practice
The style composition using array syntax [StyleSheet.absoluteFillObject, styles.fullScreenLoading, style] follows React Native best practices. The order ensures that:
- Base absolute positioning is applied first
- Theme styles are applied second
- Custom styles can override anything if needed
|
|
||
| function FullScreenLoadingIndicator({style, iconSize = CONST.ACTIVITY_INDICATOR_SIZE.LARGE, testID = '', extraLoadingContext}: FullScreenLoadingIndicatorProps) { | ||
| function FullScreenLoadingIndicator({style, iconSize = CONST.ACTIVITY_INDICATOR_SIZE.LARGE, testID = ''}: FullScreenLoadingIndicatorProps) { | ||
| const styles = useThemeStyles(); |
There was a problem hiding this comment.
⚠️ PERF-4
The useThemeStyles() hook is called on every render. For a component that might be rendered frequently during loading states, consider memoizing this:
const styles = useMemo(() => useThemeStyles(), []);However, if the theme can change at runtime, the current implementation is correct as it will respond to theme changes.
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
|
This resolves the bug |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
melvin-bot
bot
requested review from
cristipaval
and removed request for
a team
|
@cristipaval Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
🚧 @Julesssss has triggered a test Expensify/App build. You can view the workflow run here. |
…t-to-loading-log Revert "[No QA] Add more context to `ActivityIndicator` logs" (cherry picked from commit 8055208) (cherry-picked to staging by Julesssss)
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
|
🚀 Cherry-picked to staging by https://github.com/Julesssss in version: 9.2.20-4 🚀
|
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.2.20-4 🚀
|
|
🚀 Cherry-picked to staging by https://github.com/Julesssss in version: 9.2.21-0 🚀
|
|
🚧 @Julesssss has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.2.21-4 🚀
|
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.2.21-4 🚀
|
3 participants
Reverts #71053 to check fix for #71548