#6649: Exclude navigation events that should not trigger page updates #7178
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #7178 +/- ##
==========================================
- Coverage 71.31% 71.29% -0.02%
==========================================
Files 1214 1214
Lines 37780 37782 +2
Branches 7086 7089 +3
==========================================
- Hits 26941 26937 -4
- Misses 10839 10845 +6 ☔ View full report in Codecov by Sentry. |
| @@ -65,8 +65,7 @@ export async function init(): Promise<void> { | |||
| initTelemetry(); | |||
| initToaster(); | |||
|
|
|||
| await handleNavigate(); | |||
| initNavigation(); | |||
There was a problem hiding this comment.
initNavigation should deal with this directly
Kinda suggested here: #7030 (comment)
| contentScriptState: getTargetState(target), | ||
| canInject: canInjectTab(target), | ||
| // PixieBrix has some additional constraints on which tabs can be accessed (i.e., only http/https) | ||
| canAccessTab: canAccessTab(target), |
There was a problem hiding this comment.
Instead of waiting for each, sequential request, this triggers them in parallel and logs the navigation immediately, instead of possibly out of order.
Promises are still expandable in the console.
| } | ||
|
|
||
| browser.webNavigation.onHistoryStateUpdated.addListener( | ||
| debouncedTraceNavigation, |
There was a problem hiding this comment.
I updated this to use debouncing too
| */ | ||
| const _frameHref = new Map<number, string>(); |
There was a problem hiding this comment.
The content script runs in a single frame. The frameId is always the same.
| // extension background page. | ||
| thisTarget = await getThisFrame(); | ||
| } catch (error: unknown) { | ||
| console.debug("Ignoring handleNavigate because getThisFrame failed", error); |
There was a problem hiding this comment.
If that happens, that's a setup error. We fixed this specific issue in:
Also in MV3 iframes cannot load in the background page at all.
| _frameHref.set(thisTarget.frameId, href); | ||
|
|
||
| console.debug("Handling navigation to %s", href, thisTarget); | ||
| console.debug("handleNavigate:Handling navigation to %s", href, thisTarget); |
There was a problem hiding this comment.
Easy log filtering when all the logs have the same prefix
| leading: true, | ||
| trailing: true, | ||
| maxWait: 1000, | ||
| }), |
There was a problem hiding this comment.
I copied the debouncing and its comment from the original background listeners.
| // Let the content script know about navigation from the history API. Required for handling SPA navigation | ||
| browser.webNavigation.onHistoryStateUpdated.addListener(onNavigation); | ||
| async function initNavigation(): Promise<void> { | ||
| if (!(await flagOn("navigation-trace"))) { |
There was a problem hiding this comment.
Where is this flag defined? This is the only mention in both the extension and the app. I thought this was connected to the "performanceTracing" skunkworks toggle but it's not.
Untested for this reason
There was a problem hiding this comment.
It's defined and managed in the Django Admin for App.
There was a problem hiding this comment.
It's defined and managed in the Django Admin for App.
Correct, it comes in via the /api/me/ API endpoint
|
No loom links were found in the first post. Please add one there if you'd like to it to appear on Slack. Do not edit this comment manually. |
What does this PR do?
Demo
Checklist
src/tsconfig.strictNullChecks.json(if possible)