Add debug event timeline support to example#393
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| timeline.clear() | ||
| TimelineStore.register("Test #$number", timeline) | ||
| delay(100) |
There was a problem hiding this comment.
TimelineStore
TimelineStore.register("Test #$number", timeline) is called on every test run, but TimelineStore.remove(...) is never called anywhere in the codebase. Over a long test session all test timelines (and their CopyOnWriteArrayList of TimedEvent objects) accumulate indefinitely. Add a cleanup call in the finally block of screenshotFlow in TestingUtils.kt after writeTimelineToFile:
} finally {
scope.cancel()
writeTimelineToFile(testCase, testClassName, testMethodName)
TimelineStore.remove("Test #${testCase.number}")
}Prompt To Fix With AI
This is a comment left during a code review.
Path: app/src/main/java/com/superwall/superapp/test/UITestActivity.kt
Line: 66-68
Comment:
**Timeline entries never removed from `TimelineStore`**
`TimelineStore.register("Test #$number", timeline)` is called on every test run, but `TimelineStore.remove(...)` is never called anywhere in the codebase. Over a long test session all test timelines (and their `CopyOnWriteArrayList` of `TimedEvent` objects) accumulate indefinitely. Add a cleanup call in the `finally` block of `screenshotFlow` in `TestingUtils.kt` after `writeTimelineToFile`:
```kotlin
} finally {
scope.cancel()
writeTimelineToFile(testCase, testClassName, testMethodName)
TimelineStore.remove("Test #${testCase.number}")
}
```
How can I resolve this? If you propose a fix, please make it concise.| val callerFrame = Thread.currentThread().stackTrace | ||
| .firstOrNull { frame -> | ||
| frame.methodName != "screenshotFlow" && | ||
| !frame.className.startsWith("java.") && | ||
| !frame.className.startsWith("dalvik.") && | ||
| frame.className.contains("Test") | ||
| } | ||
| val testClassName = callerFrame?.className?.substringAfterLast('.') ?: "UnknownTest" | ||
| val testMethodName = callerFrame?.methodName ?: "unknownMethod" |
There was a problem hiding this comment.
Searching for frame.className.contains("Test") will misfire if a utility class (e.g. TestingUtils itself, or AbstractTest) appears higher in the call stack. When it misfires, every test produces the same filename (UnknownTest_unknownMethod.json or the wrong name), silently overwriting earlier runs. The EventTimelineRule already uses JUnit's Description API for reliable name resolution — consider consolidating on that mechanism and dropping the stack-walking path.
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/src/androidTest/java/com/example/superapp/utils/TestingUtils.kt
Line: 99-107
Comment:
**Fragile stack-trace heuristic for test name detection**
Searching for `frame.className.contains("Test")` will misfire if a utility class (e.g. `TestingUtils` itself, or `AbstractTest`) appears higher in the call stack. When it misfires, every test produces the same filename (`UnknownTest_unknownMethod.json` or the wrong name), silently overwriting earlier runs. The `EventTimelineRule` already uses JUnit's `Description` API for reliable name resolution — consider consolidating on that mechanism and dropping the stack-walking path.
How can I resolve this? If you propose a fix, please make it concise.| private fun TimelineDetailScreen( | ||
| name: String, | ||
| timeline: EventTimeline, | ||
| onBack: () -> Unit, | ||
| ) { | ||
| val events by timeline.eventsFlow.collectAsState() | ||
| var firstSelected by remember { mutableStateOf<Int?>(null) } | ||
| var secondSelected by remember { mutableStateOf<Int?>(null) } | ||
|
|
There was a problem hiding this comment.
BackHandler — system back exits the Activity instead of returning to list
The < Back text navigates within Compose state, but pressing the Android system back button exits TimelineViewerActivity entirely rather than returning to the timeline list. Add a BackHandler inside TimelineDetailScreen:
BackHandler { onBack() }Prompt To Fix With AI
This is a comment left during a code review.
Path: app/src/main/java/com/superwall/superapp/test/TimelineViewerActivity.kt
Line: 157-165
Comment:
**Missing `BackHandler` — system back exits the Activity instead of returning to list**
The `< Back` text navigates within Compose state, but pressing the Android system back button exits `TimelineViewerActivity` entirely rather than returning to the timeline list. Add a `BackHandler` inside `TimelineDetailScreen`:
```kotlin
BackHandler { onBack() }
```
How can I resolve this? If you propose a fix, please make it concise.| val configAttr = events.firstOrNull { it.eventName == "config_attributes" } | ||
| val lastWebviewComplete = events.lastOrNull { it.eventName == "paywallWebviewLoad_complete" } | ||
| if (configAttr != null && lastWebviewComplete != null) { | ||
| lastWebviewComplete.elapsed - configAttr.elapsed | ||
| } else { | ||
| null | ||
| } |
There was a problem hiding this comment.
"config_attributes", "paywallWebviewLoad_complete", "paywallPreload_start", etc. are string literals. If the SDK renames any of these event names, the preload section will silently produce null durations with no compile-time error. Consider using the type-safe durationBetween<ConfigAttributes, PaywallWebviewLoadComplete>() API from EventTimeline, or extract these strings as named constants.
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/src/main/java/com/superwall/superapp/test/TimelineViewerActivity.kt
Line: 382-388
Comment:
**Hardcoded event name strings will silently break if SDK renames events**
`"config_attributes"`, `"paywallWebviewLoad_complete"`, `"paywallPreload_start"`, etc. are string literals. If the SDK renames any of these event names, the preload section will silently produce `null` durations with no compile-time error. Consider using the type-safe `durationBetween<ConfigAttributes, PaywallWebviewLoadComplete>()` API from `EventTimeline`, or extract these strings as named constants.
How can I resolve this? If you propose a fix, please make it concise.| // Double-tap anywhere to open timeline viewer | ||
| gestureDetector = GestureDetector( | ||
| this, | ||
| object : GestureDetector.SimpleOnGestureListener() { | ||
| override fun onDoubleTap(e: MotionEvent): Boolean { | ||
| startActivity(Intent(this@MainActivity, TimelineViewerActivity::class.java)) | ||
| return true | ||
| } | ||
| }, | ||
| ) |
There was a problem hiding this comment.
The GestureDetector and TimelineViewerActivity launch are wired in the production MainActivity. While TimelineViewerActivity is android:exported="false", the gesture listener adds minor overhead and exposes developer tooling to non-debug builds. Consider wrapping the setup in if (BuildConfig.DEBUG) { … }.
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/src/main/java/com/superwall/superapp/MainActivity.kt
Line: 29-38
Comment:
**Timeline viewer gesture not gated to debug builds**
The `GestureDetector` and `TimelineViewerActivity` launch are wired in the production `MainActivity`. While `TimelineViewerActivity` is `android:exported="false"`, the gesture listener adds minor overhead and exposes developer tooling to non-debug builds. Consider wrapping the setup in `if (BuildConfig.DEBUG) { … }`.
How can I resolve this? If you propose a fix, please make it concise.
1 participant
Changes in this pull request
Checklist
CHANGELOG.mdfor any breaking changes, enhancements, or bug fixes.ktlintin the main directory and fixed any issues.Greptile Summary
This PR adds a debug event timeline system to the example app: a live
EventTimelineinMainApplication, per-test timelines inUITestInfo, a ComposeTimelineViewerActivity(reachable via double-tap), aTimelineStoreregistry, aEventTimelineRuleJUnit watcher, and Gradle helper tasks to pull/clear timeline files from the device.AndroidManifest.xmlhas a malformedandroid:sharedUserIdattribute — the closing quote appears to swallow the adjacenttools:targetApiattribute, resulting in an invalid UID value that may cause AAPT2/build errors.TimelineStoreentries registered per test are never removed, causing unbounded memory growth across long test sessions.Confidence Score: 4/5
The malformed android:sharedUserId attribute on line 32 of AndroidManifest.xml is a P0 syntax issue that may cause build failures or set an invalid UID; it must be verified/fixed before merging.
All other findings are P2 style/quality improvements. The malformed manifest attribute is the only blocking concern — it was either introduced by this PR or was pre-existing, but it's present in the current state and should be corrected. Score is 4 pending verification and fix of that single attribute.
app/src/main/AndroidManifest.xml line 32 (malformed sharedUserId attribute) requires immediate attention before merge.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[MainApplication.onCreate] -->|register 'Live'| TS[TimelineStore] A -->|liveTimeline.record| LT[EventTimeline: Live] LT --> TS UITest[UITestInfo.test] -->|timeline.clear| TT[EventTimeline: Test#N] UITest -->|register 'Test#N'| TS UITest -->|delegate.handleSuperwallEvent| TT TS -->|timelinesFlow| TVA[TimelineViewerActivity] TVA --> TLS[TimelineListScreen] TLS -->|tap| TDS[TimelineDetailScreen] TDS -->|long-press events| SEL[Duration Selection] TT -->|EventTimelineRule / screenshotFlow| WF[writeTimelineToFile] WF -->|External Storage| JSON[/sdcard/Download/superwall-event-timelines/*.json] JSON -->|adb pull Gradle task| OUT[build/outputs/event-timelines/]Comments Outside Diff (2)
app/src/main/AndroidManifest.xml, line 32-33 (link)android:sharedUserIdattributeThe closing quote of
android:sharedUserIdappears to have swallowed thetools:targetApiattribute, producing the value"com.superwall.superapp.uid tools:targetApi=". This is an invalid package name and may cause AAPT2 to reject the manifest or silently mis-set the shared UID. It should be two separate attributes:Prompt To Fix With AI
app/src/main/AndroidManifest.xml, line 8 (link)WRITE_EXTERNAL_STORAGEmissingandroid:maxSdkVersionWithout
android:maxSdkVersion="28", lint warns and the declaration is a no-op on API 29+ (where scoped storage applies). Add the bound so intent is explicit and the warning is suppressed:Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "Add debug event timeline support to exam..." | Re-trigger Greptile
Context used: