feat: dedupe incremental mutations for mobile replay #19974
Conversation
| @@ -19,10 +20,10 @@ function spacerDiv(idSequence: Generator<number>): serializedNodeWithId { | |||
| /** | |||
| * tricky: we need to accept children because that's the interface of converters, but we don't use them | |||
| */ | |||
| export function makeStatusBar(wireframe: wireframeStatusBar, _children: serializedNodeWithId[], timestamp: number, idSequence: Generator<number>): serializedNodeWithId { | |||
| const clockId = idSequence.next().value; | |||
| export function makeStatusBar(wireframe: wireframeStatusBar, _children: serializedNodeWithId[], context: ConversionContext): ConversionResult<serializedNodeWithId> { | |||
There was a problem hiding this comment.
this signature change is the meat of what has gone into the PR
we now needed to pass multiple parameters around, and rely on that state persisting through chained calls
it was getting messy and confusing
so I pulled out the concept of the conversion having a context, and the steps in the conversion returning a result.
the context can be passed along through the results
this also opens up isolating the id generation, and will make it easier to split this big file up a little
| @@ -0,0 +1,25 @@ | |||
| import {MobileStyles} from "../mobile.types"; | |||
|
|
|||
| export interface ConversionResult<T> { | |||
There was a problem hiding this comment.
TODO: reading through this now... sometimes I say transform and sometimes I saw convert but they're the same thing 🤦
|
Size Change: 0 B Total Size: 2.21 MB ℹ️ View Unchanged
|
There was a problem hiding this comment.
Looks good, left a couple of comments but nothing blocking. One thing I don't think I fully understood was the screenshot in the description. Maybe a before / after would have helped because I expected to see duplicates of one component that since this change now correctly renders one.
| attributes: inputAttributes(wireframe), | ||
| id: wireframe.id, | ||
| childNodes: [ | ||
| ...// TODO this won't work once we're editing the context |
There was a problem hiding this comment.
Just commenting to make sure you didn't miss this
There was a problem hiding this comment.
commenting here for the thread
One thing I don't think I fully understood was the screenshot in the description.
Sorry, yep, the "image" button was below the text area before because the mutations were applying multiple times but not an equal amount of multiple times so the order was changed. I didn't have a screenshot to hand 🙈
| @@ -77,8 +77,9 @@ function* ids(): Generator<number> { | |||
| } | |||
| } | |||
|
|
|||
| // TODO this is shared for the lifetime of the page, so a very, very long-lived session could exhaust the ids | |||
| const idSequence = ids() | |||
| // TODO this is shared for the lifetime of the page, | |||
There was a problem hiding this comment.
Is this still true given we now periodically reset the value in https://github.com/PostHog/posthog/pull/19974/files?diff=unified&w=1#diff-358bfae230ac47334c43cd960c1e25c388c17a0fc613868c1924dfb5db85cc3fR1131-R1132
There was a problem hiding this comment.
ah, only if you squint 🙈
|
i'm going to merge this, since it's already better, and then follow-up with improvements |
looking at the suspect recordings this still isn't quite right but is already an improvement...
e.g. here where the image button was incorrectly below the text area it is now above it