-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
feat: Refactor loading of snapshots #20632
Merged
Merged
Changes from 25 commits
Commits
Show all changes
32 commits
Select commit
Hold shift + click to select a range
a633392
feat: Refactor loading of snapshots
benjackwhite ab797d2
Update UI snapshots for `chromium` (2)
github-actions[bot] 4d1c7a7
Update query snapshots
github-actions[bot] 6543e07
Update UI snapshots for `chromium` (2)
github-actions[bot] 3a4c063
Update query snapshots
github-actions[bot] 93353b5
Update UI snapshots for `chromium` (2)
github-actions[bot] 1a7ed41
Update UI snapshots for `chromium` (2)
github-actions[bot] ce82531
Update UI snapshots for `webkit` (2)
github-actions[bot] f53b69c
Update UI snapshots for `webkit` (2)
github-actions[bot] 2e95887
Fixes
benjackwhite 1c33af8
Fix tests
benjackwhite d0cfc9b
Fixes
benjackwhite aa37022
Update UI snapshots for `chromium` (2)
github-actions[bot] bdb7775
Fix
benjackwhite 76d4e5b
Fixes
benjackwhite a73dc1b
Update UI snapshots for `chromium` (2)
github-actions[bot] b8583de
Updated logs
benjackwhite 6623d03
Fix file size check
benjackwhite d76809a
Merge branch 'master' into fix/v3-snapshots
benjackwhite cce7b1e
Update UI snapshots for `webkit` (2)
github-actions[bot] 78f9067
Update UI snapshots for `webkit` (2)
github-actions[bot] 079a6cb
Update UI snapshots for `chromium` (1)
github-actions[bot] 4fd09e5
Update UI snapshots for `webkit` (2)
github-actions[bot] 9c8b11e
Update UI snapshots for `chromium` (1)
github-actions[bot] 6d126ff
Merge branch 'master' into fix/v3-snapshots
daibhin 7d75dc8
minor fixes from review
daibhin 64f0e01
Merge branch 'master' into fix/v3-snapshots
daibhin 60447d4
Merge branch 'master' into fix/v3-snapshots
daibhin 242fdd6
remove etag changes
daibhin e3307f4
remove unnecessary line break
daibhin 713574c
fix comment
daibhin 8dc8f36
Merge branch 'master' into fix/v3-snapshots
daibhin File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,14 +30,12 @@ const BLOB_SOURCE: SessionRecordingSnapshotSource = { | |
start_timestamp: '2023-08-11T12:03:36.097000Z', | ||
end_timestamp: '2023-08-11T12:04:52.268000Z', | ||
blob_key: '1691755416097-1691755492268', | ||
loaded: false, | ||
} | ||
const REALTIME_SOURCE: SessionRecordingSnapshotSource = { | ||
source: 'realtime', | ||
start_timestamp: '2024-01-28T21:19:49.217000Z', | ||
end_timestamp: undefined, | ||
blob_key: undefined, | ||
loaded: false, | ||
} | ||
|
||
describe('sessionRecordingDataLogic', () => { | ||
|
@@ -115,11 +113,14 @@ describe('sessionRecordingDataLogic', () => { | |
it('loads all data', async () => { | ||
await expectLogic(logic, () => { | ||
logic.actions.loadRecordingMeta() | ||
logic.actions.loadRecordingSnapshots() | ||
logic.actions.loadSnapshots() | ||
}) | ||
.toDispatchActions([ | ||
'loadSnapshots', | ||
'loadSnapshotSources', | ||
'loadRecordingMetaSuccess', | ||
'loadRecordingSnapshotsSuccess', | ||
'loadSnapshotSourcesSuccess', | ||
'loadSnapshotsForSourceSuccess', | ||
'reportUsageIfFullyLoaded', | ||
]) | ||
.toFinishAllListeners() | ||
|
@@ -165,7 +166,7 @@ describe('sessionRecordingDataLogic', () => { | |
|
||
it('fetch metadata success and snapshots error', async () => { | ||
silenceKeaLoadersErrors() | ||
// Unmount and remount the logic to trigger fetching the data again after the mock change | ||
// Unmount and r emount the logic to trigger fetching the data again after the mock change | ||
logic.unmount() | ||
useMocks({ | ||
get: { | ||
|
@@ -174,9 +175,9 @@ describe('sessionRecordingDataLogic', () => { | |
}) | ||
logic.mount() | ||
logic.actions.loadRecordingMeta() | ||
logic.actions.loadRecordingSnapshots() | ||
logic.actions.loadSnapshots() | ||
|
||
await expectLogic(logic).toDispatchActions(['loadRecordingMetaSuccess', 'loadRecordingSnapshotsFailure']) | ||
await expectLogic(logic).toDispatchActions(['loadRecordingMetaSuccess', 'loadSnapshotSourcesFailure']) | ||
expect(logic.values.sessionPlayerData).toMatchObject({ | ||
person: recordingMetaJson.person, | ||
durationMs: 11868, | ||
|
@@ -219,7 +220,7 @@ describe('sessionRecordingDataLogic', () => { | |
}) | ||
|
||
await expectLogic(logic, () => { | ||
logic.actions.loadRecordingSnapshots() | ||
logic.actions.loadSnapshots() | ||
}).toDispatchActions(['loadEvents', 'loadEventsSuccess']) | ||
|
||
expect(api.create).toHaveBeenCalledWith( | ||
|
@@ -255,21 +256,21 @@ describe('sessionRecordingDataLogic', () => { | |
describe('report usage', () => { | ||
it('sends `recording loaded` event only when entire recording has loaded', async () => { | ||
await expectLogic(logic, () => { | ||
logic.actions.loadRecordingSnapshots() | ||
logic.actions.loadSnapshots() | ||
}) | ||
.toDispatchActionsInAnyOrder([ | ||
'loadRecordingSnapshots', | ||
'loadRecordingSnapshotsSuccess', | ||
'loadSnapshots', | ||
'loadSnapshotsForSourceSuccess', | ||
'loadEvents', | ||
'loadEventsSuccess', | ||
]) | ||
.toDispatchActions([eventUsageLogic.actionTypes.reportRecording]) | ||
}) | ||
it('sends `recording viewed` and `recording analyzed` event on first contentful paint', async () => { | ||
await expectLogic(logic, () => { | ||
logic.actions.loadRecordingSnapshots() | ||
logic.actions.loadSnapshots() | ||
}) | ||
.toDispatchActions(['loadRecordingSnapshotsSuccess']) | ||
.toDispatchActions(['loadSnapshotsForSourceSuccess']) | ||
.toDispatchActionsInAnyOrder([ | ||
eventUsageLogic.actionTypes.reportRecording, // loaded | ||
eventUsageLogic.actionTypes.reportRecording, // viewed | ||
|
@@ -278,7 +279,7 @@ describe('sessionRecordingDataLogic', () => { | |
}) | ||
it('clears the cache after unmounting', async () => { | ||
await expectLogic(logic, () => { | ||
logic.actions.loadRecordingSnapshots() | ||
logic.actions.loadSnapshots() | ||
}) | ||
expect(Object.keys(logic.cache)).toEqual( | ||
expect.arrayContaining(['metaStartTime', 'snapshotsStartTime', 'eventsStartTime']) | ||
|
@@ -326,7 +327,9 @@ describe('sessionRecordingDataLogic', () => { | |
}, | ||
] | ||
// we call this multiple times and pass existing data in, so we need to make sure it doesn't change | ||
expect(deduplicateSnapshots(verySimilarSnapshots, verySimilarSnapshots)).toEqual(verySimilarSnapshots) | ||
expect(deduplicateSnapshots([...verySimilarSnapshots, ...verySimilarSnapshots])).toEqual( | ||
verySimilarSnapshots | ||
) | ||
}) | ||
|
||
it('should match snapshot', () => { | ||
|
@@ -351,66 +354,68 @@ describe('sessionRecordingDataLogic', () => { | |
|
||
it('loads each source, and on success reports recording viewed', async () => { | ||
await expectLogic(logic, () => { | ||
logic.actions.loadRecordingSnapshots() | ||
// loading the snapshots will trigger a loadRecordingSnapshotsSuccess | ||
logic.actions.loadSnapshots() | ||
// loading the snapshots will trigger a loadSnapshotsForSourceSuccess | ||
// that will have the blob source | ||
// that triggers loadRecordingSnapshots | ||
// that triggers loadNextSnapshotSource | ||
}).toDispatchActions([ | ||
// the action we triggered | ||
logic.actionCreators.loadRecordingSnapshots(), | ||
'loadRecordingSnapshotsSuccess', | ||
'loadSnapshots', | ||
// the response to that triggers loading of the first item which is the blob source | ||
(action) => | ||
action.type === logic.actionTypes.loadRecordingSnapshots && | ||
action.type === logic.actionTypes.loadSnapshotsForSource && | ||
action.payload.source?.source === 'blob', | ||
'loadRecordingSnapshotsSuccess', | ||
'loadSnapshotsForSourceSuccess', | ||
// and then we report having viewed the recording | ||
'reportViewed', | ||
// the response to the success action triggers loading of the second item which is the realtime source | ||
(action) => | ||
action.type === logic.actionTypes.loadRecordingSnapshots && | ||
action.type === logic.actionTypes.loadSnapshotsForSource && | ||
action.payload.source?.source === 'realtime', | ||
'loadRecordingSnapshotsSuccess', | ||
'loadSnapshotsForSourceSuccess', | ||
// having loaded any real time data we start polling to check for more | ||
'startRealTimePolling', | ||
'pollRealtimeSnapshots', | ||
// which in turn triggers another load | ||
(action) => | ||
action.type === logic.actionTypes.loadSnapshotsForSource && | ||
action.payload.source?.source === 'realtime', | ||
'loadSnapshotsForSourceSuccess', | ||
]) | ||
}) | ||
|
||
it('can start polling for snapshots', async () => { | ||
it('polls up to a max threshold', async () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤯 I've just realised that the polling shouldn't trigger loading state in the logic - we don't want the loading affordances in the UI when we poll. Totally separate to this, reading the code back made me realise |
||
await expectLogic(logic, () => { | ||
logic.actions.startRealTimePolling() | ||
logic.actions.loadSnapshots() | ||
}) | ||
.toDispatchActions([ | ||
// the action we triggered | ||
'startRealTimePolling', | ||
'pollRecordingSnapshots', // 0 | ||
'pollRecordingSnapshotsSuccess', | ||
'loadSnapshotsForSource', // blob | ||
'loadSnapshotsForSourceSuccess', | ||
// the returned data isn't changing from our mock, | ||
// so we'll not keep polling indefinitely | ||
'pollRecordingSnapshots', // 1 | ||
'pollRecordingSnapshotsSuccess', | ||
'pollRecordingSnapshots', // 2 | ||
'pollRecordingSnapshotsSuccess', | ||
'pollRecordingSnapshots', // 3 | ||
'pollRecordingSnapshotsSuccess', | ||
'pollRecordingSnapshots', // 4 | ||
'pollRecordingSnapshotsSuccess', | ||
'pollRecordingSnapshots', // 5 | ||
'pollRecordingSnapshotsSuccess', | ||
'pollRecordingSnapshots', // 6 | ||
'pollRecordingSnapshotsSuccess', | ||
'pollRecordingSnapshots', // 7 | ||
'pollRecordingSnapshotsSuccess', | ||
'pollRecordingSnapshots', // 8 | ||
'pollRecordingSnapshotsSuccess', | ||
'pollRecordingSnapshots', // 9 | ||
'pollRecordingSnapshotsSuccess', | ||
'pollRecordingSnapshots', // 10 | ||
'pollRecordingSnapshotsSuccess', | ||
'loadSnapshotsForSource', // 1 | ||
'loadSnapshotsForSourceSuccess', | ||
'loadSnapshotsForSource', // 2 | ||
'loadSnapshotsForSourceSuccess', | ||
'loadSnapshotsForSource', // 3 | ||
'loadSnapshotsForSourceSuccess', | ||
'loadSnapshotsForSource', // 4 | ||
'loadSnapshotsForSourceSuccess', | ||
'loadSnapshotsForSource', // 5 | ||
'loadSnapshotsForSourceSuccess', | ||
'loadSnapshotsForSource', // 6 | ||
'loadSnapshotsForSourceSuccess', | ||
'loadSnapshotsForSource', // 7 | ||
'loadSnapshotsForSourceSuccess', | ||
'loadSnapshotsForSource', // 8 | ||
'loadSnapshotsForSourceSuccess', | ||
'loadSnapshotsForSource', // 9 | ||
'loadSnapshotsForSourceSuccess', | ||
'loadSnapshotsForSource', // 10 | ||
'loadSnapshotsForSourceSuccess', | ||
]) | ||
.toNotHaveDispatchedActions([ | ||
// this isn't called again | ||
'pollRecordingSnapshots', | ||
'loadSnapshotsForSource', | ||
]) | ||
|
||
await waitForExpect(() => { | ||
|
@@ -433,12 +438,14 @@ describe('sessionRecordingDataLogic', () => { | |
|
||
it('should start polling even though realtime is empty', async () => { | ||
await expectLogic(logic, () => { | ||
logic.actions.loadRecordingSnapshots() | ||
logic.actions.loadSnapshots() | ||
}).toDispatchActions([ | ||
'loadRecordingSnapshotsSuccess', | ||
'startRealTimePolling', | ||
'pollRecordingSnapshots', | ||
'pollRecordingSnapshotsSuccess', | ||
'loadSnapshots', | ||
'loadSnapshotSourcesSuccess', | ||
'loadNextSnapshotSource', | ||
'pollRealtimeSnapshots', | ||
'loadSnapshotsForSource', | ||
'loadSnapshotsForSourceSuccess', | ||
]) | ||
}) | ||
}) | ||
|
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've shipped some smaller parts of this change in previous PRs and did a thorough review today. It's pretty difficult to review 100% but I'm fairly confident it does what it's intended to. My plan is to test it locally but think it's probably worth shipping at this point and following up with any fixes forward we need.
@benjackwhite is there anything you'd like to do before we ship? There's still a few TODOs knocking about
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just don't have any spare capacity for it so its fully in your hands