Hoist error tracking to composite level#3286
Conversation
JamesBurnside
requested review from
AmyL219,
JoshuaLai,
Leah-Xia-Microsoft,
PorterNan,
alkwa-msft,
carocao-msft,
dmceachernmsft,
edwardlee-msft,
emlynmac,
jimchou-dev,
jpeng-ms,
mgamis-msft,
palatter,
prabhjot-msft,
vhuseinova-msft and
waynemo
Chat bundle size is increased❗.
|
CallWithChat bundle size is increased❗.
|
Calling bundle size is increased❗.
|
|
Failed to pass the UI Test. If this PR is for UI change and the error is snapshot mismatch, please add "update_snapshots" label to the PR for updating the snapshot. |
| const initialState = defaultMockCallAdapterState(); | ||
| initialState.latestErrors = { | ||
| 'Call.unmute': { | ||
| timestamp: new Date(Date.now() + 3600 * 1000 * 24), |
There was a problem hiding this comment.
this change was necessary because the error tracking checks the timestamp when the error was dismissed, these mock errors were set way in the future so the dismissedAt timestamp was way earlier than this mock timestamp.
| } | ||
|
|
||
| /** @private */ | ||
| export type TrackedErrors = Partial<Record<ErrorType, ErrorTrackingInfo>>; |
There was a problem hiding this comment.
Not requesting a change here but we should think about the typing here. We had a ARB comment recently around types defined as { [key: string]: Object } that is just essentially just a Record<string, Object>. do we know which typing we want to align with to not confuse the ARB? (We currently observe the former)
There was a problem hiding this comment.
Great point, no idea on this. Do you have a link to the apiview comment? They should update the azure policies if they have a strong preference but I'm also curious what the difference is!
(This is private so at least doesn't matter for this PR but is a good callout to highlight for everyone across the repo)
| const activeErrors = usePropsFor(ErrorBar).activeErrorMessages; | ||
| const [trackedErrors, setTrackedErrors] = useState<TrackedErrors>({} as TrackedErrors); | ||
| useEffect(() => { | ||
| setTrackedErrors((prev) => updateTrackedErrorsWithActiveErrors(prev, activeErrors)); |
There was a problem hiding this comment.
What do the tracked errors signify and are these internal to the call composite. Shouldn't we only have active and dismissed errors maintained globally?
There was a problem hiding this comment.
TrackedErrors is the mapping of any errors that have been triggered to information about the timestamp when they were last active, and the timestamp when it was last dismissed. trackedErrors maps the error type to an object containing:
mostRecentlyActive: Date;
lastDismissedAt?: Date;
So, for example, when a new error comes in, that error will be added to set:
trackedErrors = {
newError1: {
lastActive: Date.now()
}
}
Then if another error comes in:
trackedErrors = {
newError1: {
lastActive: <oldTimestampA>
},
newError2: {
lastActive: Date.now()
}
}
Then if error1 is dismissed but is still active (e.g. no headphones are still plugged in but the error was dismissed):
trackedErrors = {
newError1: {
lastActive: <oldTimestampA>,
dismissedAt: Date.now()
},
newError2: {
lastActive: <oldTimeStampB>
}
}
If error1 is resolved and no longer exists in stateful, its removed from the set of errors being tracked:
trackedErrors = {
newError2: {
lastActive: <oldTimeStampB>
}
}
If it reappears again, it gets added as normal, and will reappear to the user to be dismissed again:
trackedErrors = {
newError1: {
lastActive: Date.now()
},
newError2: {
lastActive: <oldTimeStampB>
}
}
There was a problem hiding this comment.
The tracking is local to the CallComposite, if someone plugged the same adapter into a new CallComposite the errors would reappear to be dimissed (an argument for moving error tracking to adapter layer but that can be a future improvement)
| <VideoEffectsPaneContent | ||
| onDismissError={onDismissVideoEffectError} | ||
| activeVideoEffectError={activeVideoEffectError} | ||
| setActiveVideoEffect={setActiveVideoEffect} |
There was a problem hiding this comment.
We have removed set active video effects, don't we need that information for the pane anymore?
There was a problem hiding this comment.
It's a good point, currently it is only used to track errors, so when I removed the error tracking code it was dead code so I removed it
There was a problem hiding this comment.
Looking into it, the VideoEffectsPaneContent tracks everything internally so yep not needed
4 participants
What
Hoist error dismissed tracking to composite level
Why
We had two issues:
How Tested
Locally:
howtested-errchanges.mp4