Conversation
There was a problem hiding this comment.
Pull request overview
Refactors the Android session replay capture pipeline to perform view traversal, filtering, and decoration (screen bounds + keyboard overlay) in a single main-thread pass, removing the previous two-stage parser+filter+decorations flow.
Changes:
- Consolidates capture output into a single
List<ReplayRect>produced byReplayParser(now also adds screen bounds + keyboard overlays and dropsIgnoreelements). - Removes
ReplayFilter,ReplayDecorations, and theFilteredCapturetypealias; moves “identical frame” suppression intoReplayCaptureEngine. - Updates public/internal callback and encoder signatures from
FilteredCapturetoList<ReplayRect>and adjusts tests/targets accordingly.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| platform/jvm/replay/src/test/kotlin/io/bitdrift/capture/replay/ReplayFilterTest.kt | Removes unit tests tied to the deleted ReplayFilter. |
| platform/jvm/replay/src/main/kotlin/io/bitdrift/capture/replay/ReplayPreviewClient.kt | Updates callback type to List<ReplayRect>. |
| platform/jvm/replay/src/main/kotlin/io/bitdrift/capture/replay/IReplayLogger.kt | Updates public callback signature to List<ReplayRect>. |
| platform/jvm/replay/src/main/kotlin/io/bitdrift/capture/replay/internal/ReplayParser.kt | Single-pass traversal now emits final replay rect list (includes screen bounds + IME overlays; filters Ignore). |
| platform/jvm/replay/src/main/kotlin/io/bitdrift/capture/replay/internal/ReplayFilter.kt | Deletes the old filter stage and FilteredCapture typealias. |
| platform/jvm/replay/src/main/kotlin/io/bitdrift/capture/replay/internal/ReplayEncoder.kt | Updates encoder input type to List<ReplayRect>. |
| platform/jvm/replay/src/main/kotlin/io/bitdrift/capture/replay/internal/ReplayDecorations.kt | Deletes the old decoration stage (screen + keyboard overlays). |
| platform/jvm/replay/src/main/kotlin/io/bitdrift/capture/replay/internal/ReplayCaptureEngine.kt | Wires new parser output directly into encoding; adds “identical capture” suppression locally. |
| platform/jvm/gradle-test-app/src/androidTest/java/io/bitdrift/gradletestapp/TestUtils.kt | Updates test logger plumbing to use List<ReplayRect>. |
| platform/jvm/gradle-test-app/src/androidTest/java/io/bitdrift/gradletestapp/ComposeReplayTest.kt | Updates test types to use List<ReplayRect>. |
| platform/jvm/gradle-test-app/src/androidTest/java/io/bitdrift/gradletestapp/AndroidViewReplayTest.kt | Updates test types to use List<ReplayRect>. |
| platform/jvm/capture/src/main/kotlin/io/bitdrift/capture/events/SessionReplayTarget.kt | Updates target callback signature to List<ReplayRect>. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| result.add(viewMapper.mapView(currentNode)) | ||
|
|
||
| // 3. Map view and filter out Ignored types (addressing TODO in ReplayFilter) |
| private fun filter(capture: List<ReplayRect>): List<ReplayRect>? { | ||
| // This capture is identical to the previous one, filter it out | ||
| return if (capture == previousCapture) { | ||
| null | ||
| } else { | ||
| previousCapture = capture | ||
| capture | ||
| } | ||
| } |
Size Comparison Report (x86_64)
|
Android Benchmark Results
Allocations
Timing
|
…re-sdk into murki/replay-refactor
|
|
||
| private fun filter(capture: List<ReplayRect>): List<ReplayRect>? { | ||
| // This capture is identical to the previous one, filter it out | ||
| return if (capture == previousCapture) { |
There was a problem hiding this comment.
Before we were ignoring when the app is backgrounded
From main:
// This capture is identical to the previous one, or is empty, filter it out
// One interesting case when capture is empty is when the application is backgrounded.
return if (filteredCapture == previousCapture || filteredCapture.isEmpty()) {
null
} else {
previousCapture = filteredCapture
filteredCapture
}
There was a problem hiding this comment.
yeah good point, this might need more testing
| private val replayEncoder: ReplayEncoder = ReplayEncoder(), | ||
| private val clock: IClock = DefaultClock.getInstance(), | ||
| ) { | ||
| private var previousCapture: List<ReplayRect>? = null |
There was a problem hiding this comment.
Looks like this is only accessed from the background thread so probably ok for now to not make it thread safe
|
|
||
| internal typealias Capture = List<List<ReplayRect>> | ||
|
|
||
| internal class ReplayParser( |
There was a problem hiding this comment.
Wondering if we should rename this now as is doing more things now than just parsing (e.g. computeDisplayRect(), compute IME overlay, etc)
| filter(timedValue.value)?.let { filteredCapture -> | ||
| replayCaptureMetrics.parseDuration = timedValue.duration | ||
| replayCaptureMetrics.viewCountAfterFilter = filteredCapture.size | ||
| val screen = captureDecorations.addDecorations(filteredCapture) |
There was a problem hiding this comment.
This was happening in background thread, we should measure the impact on jank for this change (I can take a look)
There was a problem hiding this comment.
yeah in particular there will be 1 extra check for every root view to look for the keyboard, I'd expect it to be very low overhead but probably not 0
|
Will take a look this week |
Main changes in
ReplayCaptureEngineandReplayParser.Before the traversal was split in two:
Now it's been condensed in the single first pass. Also deleted a bunch of confusing extra classes (
ReplayDecoration,ReplayFilter,FilteredCapturetypealias).Updated
examples/android/MainActivity.ktto use the newList<ReplayRect>signature in theonScreenCapturedoverride, replacing the removedFilteredCapturetypealias.test session: https://timeline.bitdrift.dev/session/f21546b4-95de-485c-a9a1-ceb485292d99
CHANGELOG.md's "Unreleased" section has been updated, if applicable.