Skip to content

Add screen_view and screen_leave analytic events #116

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Aug 20, 2025
Merged

Conversation

hamorillo
Copy link
Contributor

Description

This PR adds the properties to the Event class in the analytics module. Also, to verify that it works, I'm adding the screen_view and screen_leave events on the Screen composable.

This way, we can also work in parallel to add analytics, avoiding some conflicts.

Testing Steps

  • Verify you can see the events on tracks (you can use the filter gravatarandroid%)
  • You can also verify that the event is tracked by adding a flush after each event and analyzing the network requests.

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds analytics tracking for screen view and leave events to the app. It enhances the Event class to support properties and implements automatic screen tracking through the Screen composable.

  • Added properties field to Event interface with JSON serialization support
  • Created DesignEvent sealed class with ScreenView and ScreenLeave events
  • Modified Screen composable to automatically track screen view/leave events with LaunchedEffect and DisposableEffect

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
analytics/src/main/kotlin/com/gravatar/analytics/Event.kt Added properties interface to Event for JSON serialization
design/src/main/kotlin/com/gravatar/app/design/analytics/DesignEvent.kt New sealed class defining screen tracking events with properties
design/src/main/kotlin/com/gravatar/app/design/components/Screen.kt Modified to accept screenName parameter and automatically track screen events
analytics/src/main/kotlin/com/gravatar/analytics/tracks/TracksTracker.kt Updated to include event properties and prefix in track calls
design/build.gradle.kts Added analytics module dependency and Koin libraries
analytics/src/test/kotlin/com/gravatar/analytics/tracks/TrackTrackerTest.kt Updated test assertions to match new TracksTracker behavior
loginUi/src/main/kotlin/com/gravatar/app/loginUi/presentation/login/LoginScreen.kt Added screenName parameter to Screen call
homeUi/src/main/kotlin/com/gravatar/app/homeUi/presentation/home/share/ShareScreen.kt Added screenName parameter to Screen call
homeUi/src/main/kotlin/com/gravatar/app/homeUi/presentation/home/profile/ProfileScreen.kt Added screenName parameter to Screen call
homeUi/src/main/kotlin/com/gravatar/app/homeUi/presentation/home/gravatar/GravatarScreen.kt Added screenName parameter to Screen call

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.


@Composable
fun Screen(
screenName: String,
Copy link
Preview

Copilot AI Aug 19, 2025

Choose a reason for hiding this comment

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

Adding a required screenName parameter is a breaking change to the Screen composable API. Consider making this parameter optional with a default value to maintain backward compatibility, or provide an overloaded function that doesn't require the screenName parameter.

Suggested change
screenName: String,
screenName: String = "",

Copilot uses AI. Check for mistakes.

@@ -6,14 +6,20 @@ import com.gravatar.analytics.Tracker
import java.util.UUID

internal class TracksTracker(private val tracksClient: TracksClient) : Tracker() {

internal companion object {
const val TRACKS_EVENT_NAME_PREFIX = "gravatarandroid_"
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ Is that a Track's requirement to prefix all the events?y

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I remember, yes. In fact, we should use that concrete prefix as it was added to the allow list.

Copy link
Contributor

@AdamGrzybkowski AdamGrzybkowski left a comment

Choose a reason for hiding this comment

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

:shipit:

@hamorillo hamorillo merged commit 62709b1 into trunk Aug 20, 2025
10 checks passed
@hamorillo hamorillo deleted the hamorillo/GRA-752 branch August 20, 2025 07:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants