From 2e327c9d0c9ec9768d00a2eed342578a62071440 Mon Sep 17 00:00:00 2001 From: Andrey Lushnikov Date: Tue, 6 Jun 2023 17:38:44 -0700 Subject: [PATCH] fix: miscellaneous improvements for tracing UI (#23558) - feat(tracing): mark API requests with "API" label - feat(tracing): do not attribute any resources to `route.` API calls; otherwise, network traffic might get inside the `route.` actions. - fix(tracing): map actionIds from primary contexts to actionIds from non-primary contexts - fix(tracing): show leading `/` in URL path in network panel This is a result of a pair-programming session with @pavelfeldman --- packages/playwright-core/src/server/har/harTracer.ts | 1 + packages/trace-viewer/src/ui/modelUtil.ts | 12 ++++++++++-- .../trace-viewer/src/ui/networkResourceDetails.css | 7 +++++++ .../trace-viewer/src/ui/networkResourceDetails.tsx | 6 ++++-- packages/trace/src/har.ts | 1 + tests/library/trace-viewer.spec.ts | 10 +++++----- 6 files changed, 28 insertions(+), 9 deletions(-) diff --git a/packages/playwright-core/src/server/har/harTracer.ts b/packages/playwright-core/src/server/har/harTracer.ts index 31536d22c9..36f7d8ede2 100644 --- a/packages/playwright-core/src/server/har/harTracer.ts +++ b/packages/playwright-core/src/server/har/harTracer.ts @@ -196,6 +196,7 @@ export class HarTracer { if (!this._shouldIncludeEntryWithUrl(event.url.toString())) return; const harEntry = createHarEntry(event.method, event.url, undefined, this._options); + harEntry._apiRequest = true; if (!this._options.omitCookies) harEntry.request.cookies = event.cookies; harEntry.request.headers = Object.entries(event.headers).map(([name, value]) => ({ name, value })); diff --git a/packages/trace-viewer/src/ui/modelUtil.ts b/packages/trace-viewer/src/ui/modelUtil.ts index a86be35393..e895e4fd05 100644 --- a/packages/trace-viewer/src/ui/modelUtil.ts +++ b/packages/trace-viewer/src/ui/modelUtil.ts @@ -86,7 +86,13 @@ function indexModel(context: ContextEntry) { for (let i = 0; i < context.actions.length; ++i) { const action = context.actions[i] as any; action[contextSymbol] = context; - action[nextInContextSymbol] = context.actions[i + 1]; + } + let lastNonRouteAction = undefined; + for (let i = context.actions.length - 1; i >= 0; i--) { + const action = context.actions[i] as any; + action[nextInContextSymbol] = lastNonRouteAction; + if (!action.apiName.includes('route.')) + lastNonRouteAction = action; } for (const event of context.events) (event as any)[contextSymbol] = context; @@ -109,6 +115,7 @@ function mergeActions(contexts: ContextEntry[]) { offset = context.actions[0].startTime - context.actions[0].wallTime; } + const nonPrimaryIdToPrimaryId = new Map(); for (const context of nonPrimaryContexts) { for (const action of context.actions) { if (offset) { @@ -122,12 +129,13 @@ function mergeActions(contexts: ContextEntry[]) { const key = `${action.apiName}@${action.wallTime}`; const existing = map.get(key); if (existing && existing.apiName === action.apiName) { + nonPrimaryIdToPrimaryId.set(action.callId, existing.callId); if (action.error) existing.error = action.error; if (action.attachments) existing.attachments = action.attachments; if (action.parentId) - existing.parentId = action.parentId; + existing.parentId = nonPrimaryIdToPrimaryId.get(action.parentId) ?? action.parentId; continue; } map.set(key, { ...action, context }); diff --git a/packages/trace-viewer/src/ui/networkResourceDetails.css b/packages/trace-viewer/src/ui/networkResourceDetails.css index 4185456dae..3846e37c49 100644 --- a/packages/trace-viewer/src/ui/networkResourceDetails.css +++ b/packages/trace-viewer/src/ui/networkResourceDetails.css @@ -39,6 +39,8 @@ border-radius: 4px; margin: 2px; line-height: 20px; + min-width: 30px; + text-align: center; } .network-request-title-status.status-failure { @@ -51,6 +53,11 @@ background-color: var(--vscode-statusBar-background); } +.network-request-title-status.status-route.api { + color: var(--vscode-statusBar-foreground); + background-color: var(--vscode-statusBarItem-remoteBackground); +} + .network-request-title-url { overflow: hidden; text-overflow: ellipsis; diff --git a/packages/trace-viewer/src/ui/networkResourceDetails.tsx b/packages/trace-viewer/src/ui/networkResourceDetails.tsx index 84619874c4..0e7fba8f6c 100644 --- a/packages/trace-viewer/src/ui/networkResourceDetails.tsx +++ b/packages/trace-viewer/src/ui/networkResourceDetails.tsx @@ -69,7 +69,7 @@ export const NetworkResourceDetails: React.FunctionComponent<{ const routeStatus = formatRouteStatus(resource); const requestContentTypeHeader = resource.request.headers.find(q => q.name === 'Content-Type'); const requestContentType = requestContentTypeHeader ? requestContentTypeHeader.value : ''; - const resourceName = resource.request.url.substring(resource.request.url.lastIndexOf('/') + 1); + const resourceName = resource.request.url.substring(resource.request.url.lastIndexOf('/')); let contentType = resource.response.content.mimeType; const charset = contentType.match(/^(.*);\s*charset=.*$/); if (charset) @@ -79,7 +79,7 @@ export const NetworkResourceDetails: React.FunctionComponent<{ const renderTitle = React.useCallback(() => { return
- {routeStatus &&
{routeStatus}
} + {routeStatus &&
{routeStatus}
} {resource.response._failureText &&
{resource.response._failureText}
} {!resource.response._failureText &&
{resource.response.status}
}
{resource.request.method}
@@ -147,5 +147,7 @@ function formatRouteStatus(request: Entry): string { return 'continued'; if (request._wasFulfilled) return 'fulfilled'; + if (request._apiRequest) + return 'api'; return ''; } diff --git a/packages/trace/src/har.ts b/packages/trace/src/har.ts index c273dc06ce..fa9a5e4839 100644 --- a/packages/trace/src/har.ts +++ b/packages/trace/src/har.ts @@ -71,6 +71,7 @@ export type Entry = { _wasAborted?: boolean; _wasFulfilled?: boolean; _wasContinued?: boolean; + _apiRequest?: boolean; }; export type Request = { diff --git a/tests/library/trace-viewer.spec.ts b/tests/library/trace-viewer.spec.ts index 0478a4c20d..9b183e86f8 100644 --- a/tests/library/trace-viewer.spec.ts +++ b/tests/library/trace-viewer.spec.ts @@ -216,9 +216,9 @@ test('should have network requests', async ({ showTraceViewer }) => { const traceViewer = await showTraceViewer([traceFile]); await traceViewer.selectAction('http://localhost'); await traceViewer.showNetworkTab(); - await expect(traceViewer.networkRequests).toContainText([/200GETframe.htmltext\/html/]); - await expect(traceViewer.networkRequests).toContainText([/200GETstyle.csstext\/css/]); - await expect(traceViewer.networkRequests).toContainText([/200GETscript.jsapplication\/javascript/]); + await expect(traceViewer.networkRequests).toContainText([/200GET\/frame.htmltext\/html/]); + await expect(traceViewer.networkRequests).toContainText([/200GET\/style.csstext\/css/]); + await expect(traceViewer.networkRequests).toContainText([/200GET\/script.jsapplication\/javascript/]); }); test('should have network request overrides', async ({ page, server, runAndTrace }) => { @@ -228,7 +228,7 @@ test('should have network request overrides', async ({ page, server, runAndTrace }); await traceViewer.selectAction('http://localhost'); await traceViewer.showNetworkTab(); - await expect(traceViewer.networkRequests).toContainText([/200GETframe.htmltext\/html/]); + await expect(traceViewer.networkRequests).toContainText([/200GET\/frame.htmltext\/html/]); await expect(traceViewer.networkRequests).toContainText([/abort.*style.cssx-unknown/]); }); @@ -239,7 +239,7 @@ test('should have network request overrides 2', async ({ page, server, runAndTra }); await traceViewer.selectAction('http://localhost'); await traceViewer.showNetworkTab(); - await expect(traceViewer.networkRequests).toContainText([/200GETframe.htmltext\/html/]); + await expect(traceViewer.networkRequests).toContainText([/200GET\/frame.htmltext\/html/]); await expect(traceViewer.networkRequests).toContainText([/continue.*script.jsapplication\/javascript/]); });