Skip to content
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@

### Fixes

- Fixes orientation change misalignment for session replay on Android ([#5321](https://github.com/getsentry/sentry-react-native/pull/5321))
- Sync `user.geo` from `SetUser` to the native layer ([#5302](https://github.com/getsentry/sentry-react-native/pull/5302))

### Dependencies
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package io.sentry.rnsentryandroidtester

import android.view.View
import android.view.ViewGroup
import android.view.ViewTreeObserver
import androidx.fragment.app.Fragment
import com.facebook.react.bridge.ReactContext
import com.facebook.react.uimanager.UIManagerHelper
Expand Down Expand Up @@ -84,6 +85,34 @@ class RNSentryReactFragmentLifecycleTracerTest {
callOnFragmentViewCreated(mock<ScreenStackFragment>(), mockScreenViewWithGenericContext())
}

@Test
fun tracerAttachesLayoutListener() {
val mockEventDispatcher = mock<EventDispatcher>()
val mockViewTreeObserver = mock<ViewTreeObserver>()
mockUIManager(mockEventDispatcher)

val mockView = mockScreenViewWithReactContext(mockViewTreeObserver)
callOnFragmentViewCreated(mock<ScreenStackFragment>(), mockView)

verify(mockViewTreeObserver, times(1)).addOnGlobalLayoutListener(any())
}

@Test
fun tracerRemovesLayoutListenerWhenFragmentViewDestroyed() {
val mockEventDispatcher = mock<EventDispatcher>()
val mockViewTreeObserver = mock<ViewTreeObserver>()
mockUIManager(mockEventDispatcher)

val mockFragment = mock<ScreenStackFragment>()
val mockView = mockScreenViewWithReactContext(mockViewTreeObserver)

val tracer = createSutWith()
tracer.onFragmentViewCreated(mock(), mockFragment, mockView, null)
tracer.onFragmentViewDestroyed(mock(), mockFragment)

verify(mockViewTreeObserver, times(1)).removeOnGlobalLayoutListener(any())
}

private fun callOnFragmentViewCreated(
mockFragment: Fragment,
mockView: View,
Expand All @@ -107,51 +136,58 @@ class RNSentryReactFragmentLifecycleTracerTest {
)
}

private fun mockScreenViewWithReactContext(): View {
private fun mockScreenViewWithReactContext(mockViewTreeObserver: ViewTreeObserver = mock()): View {
val screenMock: View =
mock {
whenever(it.id).thenReturn(123)
whenever(it.context).thenReturn(mock<ReactContext>())
whenever(it.viewTreeObserver).thenReturn(mockViewTreeObserver)
}
val mockView =
mock<ViewGroup> {
whenever(it.childCount).thenReturn(1)
whenever(it.getChildAt(0)).thenReturn(screenMock)
whenever(it.viewTreeObserver).thenReturn(mockViewTreeObserver)
}
return mockView
}

private fun mockScreenViewWithGenericContext(): View {
private fun mockScreenViewWithGenericContext(mockViewTreeObserver: ViewTreeObserver = mock()): View {
val screenMock: View =
mock {
whenever(it.id).thenReturn(123)
whenever(it.context).thenReturn(mock())
whenever(it.viewTreeObserver).thenReturn(mockViewTreeObserver)
}
val mockView =
mock<ViewGroup> {
whenever(it.childCount).thenReturn(1)
whenever(it.getChildAt(0)).thenReturn(screenMock)
whenever(it.viewTreeObserver).thenReturn(mockViewTreeObserver)
}
return mockView
}

private fun mockScreenViewWithNoId(): View {
private fun mockScreenViewWithNoId(mockViewTreeObserver: ViewTreeObserver = mock()): View {
val screenMock: View =
mock {
whenever(it.id).thenReturn(-1)
whenever(it.context).thenReturn(mock<ReactContext>())
whenever(it.viewTreeObserver).thenReturn(mockViewTreeObserver)
}
val mockView =
mock<ViewGroup> {
whenever(it.childCount).thenReturn(1)
whenever(it.getChildAt(0)).thenReturn(screenMock)
whenever(it.viewTreeObserver).thenReturn(mockViewTreeObserver)
}
return mockView
}

private fun mockScreenViewWithoutChild(): View =
private fun mockScreenViewWithoutChild(mockViewTreeObserver: ViewTreeObserver = mock()): View =
mock<ViewGroup> {
whenever(it.childCount).thenReturn(0)
whenever(it.viewTreeObserver).thenReturn(mockViewTreeObserver)
}

private fun mockUIManager(mockEventDispatcher: EventDispatcher) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import android.os.Bundle;
import android.view.View;
import android.view.ViewGroup;
import android.view.ViewTreeObserver;
import androidx.annotation.NonNull;
import androidx.fragment.app.Fragment;
import androidx.fragment.app.FragmentManager;
Expand All @@ -13,9 +14,13 @@
import com.facebook.react.uimanager.events.EventDispatcher;
import com.facebook.react.uimanager.events.EventDispatcherListener;
import io.sentry.ILogger;
import io.sentry.Integration;
import io.sentry.ScopesAdapter;
import io.sentry.SentryLevel;
import io.sentry.SentryOptions;
import io.sentry.android.core.BuildInfoProvider;
import io.sentry.android.core.internal.util.FirstDrawDoneListener;
import io.sentry.android.replay.ReplayIntegration;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;

Expand All @@ -25,6 +30,13 @@ public class RNSentryReactFragmentLifecycleTracer extends FragmentLifecycleCallb
private @NotNull final Runnable emitNewFrameEvent;
private @NotNull final ILogger logger;

private @Nullable ReplayIntegration replayIntegration;
private int lastWidth = -1;
private int lastHeight = -1;

private @Nullable View currentView;
private @Nullable ViewTreeObserver.OnGlobalLayoutListener currentListener;

public RNSentryReactFragmentLifecycleTracer(
@NotNull BuildInfoProvider buildInfoProvider,
@NotNull Runnable emitNewFrameEvent,
Expand Down Expand Up @@ -95,6 +107,94 @@ public void onEventDispatch(Event event) {
}
}
});

// Add layout listener to detect configuration changes after detaching any previous one
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so, as I understood this logic only applies when ReactNavigationIntegration is used and ttid-tracking is enabled. I was wondering if we should actually make it independent from the both above, and just introduce a replay-specific FragmentLifecycleCallbacks? Also, is this only a problem when fragments are used (i.e. reactNavigation), or also applies to activity-based apps?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch @romtsn 🙇

make it independent from the both above, and just introduce a replay-specific FragmentLifecycleCallbacks?

Makes sense 👍 Updated with bbddc27

Also, is this only a problem when fragments are used (i.e. reactNavigation), or also applies to activity-based apps?

I was able to still reproduce the issue on a bare app that seems to have only Activities. I think for most real world scenarios the fragment implementation would be used and I would opt for handling this case separately.

Example Activity only app

import React from 'react';
import { Button, View, Text, StyleSheet } from 'react-native';
import * as Sentry from '@sentry/react-native';

Sentry.init({
  dsn: 'DSN',

  // Adds more context data to events (IP address, cookies, user, etc.)
  // For more information, visit: https://docs.sentry.io/platforms/react-native/data-management/data-collected/
  sendDefaultPii: true,

  // Enable Logs
  enableLogs: true,

  // Configure Session Replay
  replaysSessionSampleRate: 0.1,
  replaysOnErrorSampleRate: 1,
  integrations: [Sentry.mobileReplayIntegration(), Sentry.feedbackIntegration()],

  // uncomment the line below to enable Spotlight (https://spotlightjs.com)
  // spotlight: __DEV__,
});

export default function App() {
  return (
    <View style={styles.container}>
      <Text style={styles.text}>Hello, Minimal App!</Text>
      <Button title='Try!' onPress={ () => { Sentry.captureException(new Error('First error')) }}/>
    </View>
  );
}

const styles = StyleSheet.create({
  container: {
    flex: 1,
    justifyContent: 'center',
    alignItems: 'center',
    backgroundColor: '#fff',
  },
  text: {
    fontSize: 20,
  },
});

export default Sentry.wrap(App);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, let's not care about it for now. If we get reports we can do that later 👍 Let me review the updated logic

detachLayoutChangeListener();
attachLayoutChangeListener(v);
}

@Override
public void onFragmentViewDestroyed(@NonNull FragmentManager fm, @NonNull Fragment f) {
detachLayoutChangeListener();
}

private void attachLayoutChangeListener(final View view) {
final ViewTreeObserver.OnGlobalLayoutListener listener =
new ViewTreeObserver.OnGlobalLayoutListener() {
@Override
public void onGlobalLayout() {
checkAndNotifyWindowSizeChange(view);
}
};

currentView = view;
currentListener = listener;

view.getViewTreeObserver().addOnGlobalLayoutListener(listener);
}

private void detachLayoutChangeListener() {
if (currentView != null && currentListener != null) {
try {
currentView.getViewTreeObserver().removeOnGlobalLayoutListener(currentListener);
} catch (Exception e) {
logger.log(SentryLevel.DEBUG, "Failed to remove layout change listener", e);
}
}

currentView = null;
currentListener = null;
}

private void checkAndNotifyWindowSizeChange(View view) {
try {
android.util.DisplayMetrics metrics = view.getContext().getResources().getDisplayMetrics();
int currentWidth = metrics.widthPixels;
int currentHeight = metrics.heightPixels;

if (lastWidth != currentWidth || lastHeight != currentHeight) {
lastWidth = currentWidth;
lastHeight = currentHeight;

notifyReplayIntegrationOfSizeChange(currentWidth, currentHeight);
}
} catch (Exception e) {
logger.log(SentryLevel.DEBUG, "Failed to check window size", e);
}
}

private void notifyReplayIntegrationOfSizeChange(int width, int height) {
try {
if (replayIntegration == null) {
replayIntegration = getReplayIntegration();
}

if (replayIntegration == null) {
return;
}

replayIntegration.onWindowSizeChanged(width, height);
} catch (Exception e) {
logger.log(SentryLevel.DEBUG, "Failed to notify replay integration of size change", e);
}
}

private @Nullable ReplayIntegration getReplayIntegration() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think you could simplify this method by calling ScopesAdapter.getInstance().getOptions().getReplayController() and then checking if it's instanceof ReplayIntegration. This would spare a few cycles.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, we could even expose the onWindowSizeChanged method in the ReplayController interface, but would require a new Android SDK release, so I'll leave it up to you to decide :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea @romtsn 🙇
Updated with dbee1d4 and verified that everything works as before.

Alternatively, we could even expose the onWindowSizeChanged method in the ReplayController interface, but would require a new Android SDK release, so I'll leave it up to you to decide :)

I'll add a note to iterate on this as an improvement. It would be nice to ship the fix soon since this is also a PII.

try {
final SentryOptions options = ScopesAdapter.getInstance().getOptions();
if (options == null) {
return null;
}

for (Integration integration : options.getIntegrations()) {
if (integration instanceof ReplayIntegration) {
return (ReplayIntegration) integration;
}
}
} catch (Exception e) {
logger.log(SentryLevel.DEBUG, "Error getting replay integration", e);
}
return null;
}

private static @Nullable EventDispatcher getEventDispatcherForReactTag(
Expand Down
Loading