Fix breakage report race condition. #2939
Conversation
Config processing can how take one tick after the config ready promise resolves. Waiting for allLoadingFinished should ensure that everything is ready.
| @@ -321,6 +321,8 @@ export async function breakageReportForTab({ | |||
| export async function sendBreakageReportForCurrentTab({ pixelName, currentTab, category, description, reportFlow }) { | |||
| await settings.ready(); | |||
| await tdsStorage.ready('config'); | |||
There was a problem hiding this comment.
Any reason to wait for both tdsStorage.ready('config') and tdsStorage.config.allLoadingFinished?
sammacbeth
force-pushed
the
sam/breakage-config-version
branch
from
a8bc2fe to
7f4d812
Compare
| 1, | ||
| 2, | ||
| ); | ||
| await expectReports([[{}, 'protection-toggled-off-breakage-report', 'on_protections_off_dashboard_main']], 1, 2); |
There was a problem hiding this comment.
I'd like to keep the test coverage here for the actual pixels being sent.
There was a problem hiding this comment.
I see your point, but what would that actually test? It would actually just be a test of dashboardMessaging.submitBrokenSiteReport, that it passes the arguments given here correctly to the pixel request. That would be better encapsulated in a separate unit-test.
Part of the advantage of this refactor, is that we can mock the boundaries of ToggleReport better - rather than having to intercept the outgoing pixel to test that a report was sent, we can just mock the interface for sending reports.
There was a problem hiding this comment.
Note, I've added some new tests for dashboardMessaging.submitBrokenSiteReport explicitly, so I think everything should be now covered. The toggle-reports tests test everything up to the submitBrokenSiteReport call, then the dashboard-messaging tests check that call to pixel sent.
| */ | ||
| async submitBrokenSiteReport(breakageReport, pixelName = 'epbf', reportFlow = undefined) { | ||
| // wait for config and TDS so we can get etags and config version | ||
| await Promise.all([this.tds.remoteConfig.allLoadingFinished, this.tds.tds.ready]); |
There was a problem hiding this comment.
Is it just this await Promise.all([this.tds.remoteConfig.allLoadingFinished, this.tds.tds.ready]); that's resolving the race condition, or are there other changes here that do too? I'm thinking we should split this into two PRs, one for the race condition and one for the refactor.
| /** | ||
| * Message handlers for communication from the dashboard to the extension background. | ||
| * | ||
| * Note, additional message handles for toggle reports is in the separate ToggleReports component. |
There was a problem hiding this comment.
I think there are other dashboard messages that we didn't move here? E.g. for the fire button. Maybe we should mention some of those in the comment here, or move the handlers into this component too? I think the docs here might help https://duckduckgo.github.io/privacy-dashboard/modules/Browser_Extensions_integration.html
There was a problem hiding this comment.
I've updated that docstring to include all messages and where they are being handled. Thanks for pointing to those docs, super helpful!
Co-authored-by: Dave Vandyke <[email protected]>
Reviewer:
Description:
Some breakage reports were coming through with config version
undefinedon Firefox (https://app.asana.com/0/0/1209244730607066/f). This is likely caused by a race condition in the breakage report flow if the background is not fully loaded yet.To fix this, this PR:
Steps to test this PR:
Automated tests:
Reviewer Checklist:
PR Author Checklist: