fix(replay): wait for new config before recording decision#3128
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
ksvat
force-pushed
the
wait-for-new-config
branch
from
3f7b2e6 to
4ab2dc8
Compare
Contributor
ksvat
changed the title
ksvat
requested review from
a team,
TueHaulund and
fasyy612
and removed request for
a team
ksvat
force-pushed
the
wait-for-new-config
branch
from
4ab2dc8 to
f7a17a7
Compare
Contributor
|
Size Change: +778 B (+0.01%) Total Size: 6.69 MB
ℹ️ View Unchanged
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+1337
to
+1338
| // If still buffering (waiting for remote config), discard the buffer | ||
| // Better to lose this data than record incorrectly. |
Contributor
There was a problem hiding this comment.
misleading comment - at this point, remote config has already been received (otherwise the lazy recorder wouldn't exist). BUFFERING here means waiting for a URL/event trigger to match, not waiting for remote config. The guard against starting before remote config is in session-recording.ts:78.
Suggested change
| // If still buffering (waiting for remote config), discard the buffer | |
| // Better to lose this data than record incorrectly. | |
| // If still buffering (waiting for URL/event trigger), discard the buffer | |
| // Better to lose this data than record incorrectly. |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/browser/src/extensions/replay/external/lazy-loaded-session-recorder.ts
Line: 1337-1338
Comment:
misleading comment - at this point, remote config has already been received (otherwise the lazy recorder wouldn't exist). `BUFFERING` here means waiting for a URL/event trigger to match, not waiting for remote config. The guard against starting before remote config is in `session-recording.ts:78`.
```suggestion
// If still buffering (waiting for URL/event trigger), discard the buffer
// Better to lose this data than record incorrectly.
```
How can I resolve this? If you propose a fix, please make it concise.| expect(sessionRecording.status).toBe('active') | ||
| }) | ||
|
|
||
| it('discards buffer on beforeunload if remote config not yet loaded', () => { |
Contributor
There was a problem hiding this comment.
test name is misleading - remote config IS loaded on line 417. The test actually verifies that the buffer is cleared when in BUFFERING status (waiting for URL trigger) during beforeunload.
Suggested change
| it('discards buffer on beforeunload if remote config not yet loaded', () => { | |
| it('discards buffer on beforeunload when waiting for url trigger to match', () => { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/browser/src/__tests__/extensions/replay/sessionRecording-onRemoteConfig.test.ts
Line: 407
Comment:
test name is misleading - remote config IS loaded on line 417. The test actually verifies that the buffer is cleared when in `BUFFERING` status (waiting for URL trigger) during `beforeunload`.
```suggestion
it('discards buffer on beforeunload when waiting for url trigger to match', () => {
```
How can I resolve this? If you propose a fix, please make it concise.
Contributor
There was a problem hiding this comment.
@ksvat Claude also flagged this for me when reviewing the branch. We will buffer while waiting for a url trigger to match and with this PR we'll now discard that buffer on beforeunload. Doesn't seem like what we want?
Contributor
Author
There was a problem hiding this comment.
yeah i think this is leftover from before we added the new "waiting for config" state, when BUFFERING also handled that state --- thanks for the catch!!
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
packages/browser/src/extensions/replay/external/lazy-loaded-session-recorder.ts
Show resolved
Hide resolved
ksvat
force-pushed
the
wait-for-new-config
branch
2 times, most recently
from
6271f85 to
97001f2
Compare
ksvat
force-pushed
the
wait-for-new-config
branch
from
97001f2 to
91f0965
Compare
ksvat
force-pushed
the
wait-for-new-config
branch
from
91f0965 to
ff3bd62
Compare
ksvat
force-pushed
the
wait-for-new-config
branch
from
ff3bd62 to
cce08f8
Compare
packages/browser/src/extensions/replay/external/lazy-loaded-session-recorder.ts
Show resolved
Hide resolved
ksvat
force-pushed
the
wait-for-new-config
branch
from
410ff88 to
640f9aa
Compare
ksvat
force-pushed
the
wait-for-new-config
branch
from
640f9aa to
d0711dc
Compare
ksvat
force-pushed
the
wait-for-new-config
branch
from
d0711dc to
ecb8269
Compare
packages/browser/src/extensions/replay/external/lazy-loaded-session-recorder.ts
Outdated
Show resolved
Hide resolved
ksvat
force-pushed
the
wait-for-new-config
branch
from
ecb8269 to
4acb34b
Compare
packages/browser/playwright/mocked/session-recording/triggerMatch-type.spec.ts
Show resolved
Hide resolved
ksvat
force-pushed
the
wait-for-new-config
branch
from
4acb34b to
766a8de
Compare
TueHaulund
approved these changes
Feb 26, 2026
ksvat
force-pushed
the
wait-for-new-config
branch
from
766a8de to
5e37cd6
Compare
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
had a couple tickets recently where it seems like recording is starting based on a Null config even though later receiving a config that indicates a recording SHOULDNT have started.
Changes
wait for remote config before deciding to record or not
if unload before receiving commit, clear hte buffer rather than flush it
Release info Sub-libraries affected
Libraries affected
Checklist
If releasing new changes
pnpm changesetto generate a changeset file