-
Notifications
You must be signed in to change notification settings - Fork 3.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
feature(trace-viewer): embedded mode support PoC #30885
base: main
Are you sure you want to change the base?
feature(trace-viewer): embedded mode support PoC #30885
Conversation
0556550
to
f4181da
Compare
This comment has been minimized.
This comment has been minimized.
f4181da
to
b287930
Compare
This comment has been minimized.
This comment has been minimized.
I'm supportive for those, let me know when they are no longer drafts! |
@pavelfeldman I removed the draft, but this is still a very basic implementation just to check if the integration was possible. I found out some issues with this approach: when the trace viewer iframe gets the focus keyboard shortcuts stop working, and I ned to click outside it to open the command palette for instance. |
159c9ab
to
71029da
Compare
71029da
to
216aedb
Compare
This comment has been minimized.
This comment has been minimized.
That's unfortunate! I was rendering webviews in vscode w/o such issues. Is iframe w/ snapshot breaking it? How is service worker doing, are snapshots rendered fine? |
I tried to render the trace viewer html directly without iframe and service worker doesn't work there. Nevertheless, we can just rely on the running server instead of the service worker. The only thing I still need to figure out is on how to make the websocket work, or if not possible, on how to emulate it. |
Unfortunately, we can't. It is essential for SW to fulfill all the requests. |
I am figuring that out the hard way :( I was doing some quick and dirty tests and deactivated the sw to rely on the webserver. I thought it was already handling trace requests as a fallback, but it was not, so I had to copy & paste the service worker logic there with necessary adjustments for it to work, but I stumbled on cross-domain snapshot resources that are handled by the sw and I don't see a way out of that... I'll go back to the iframe solution and see if there's a workaround for it. |
Problem explained here, with a workaround proposal: |
That did the trick, key bindings are triggered as expected when trace viewer is focused |
This comment has been minimized.
This comment has been minimized.
packages/trace-viewer/src/index.tsx
Outdated
@@ -39,5 +39,26 @@ import { WorkbenchLoader } from './ui/workbenchLoader'; | |||
setInterval(function() { fetch('ping'); }, 10000); | |||
} | |||
|
|||
if (window.parent !== window && new URL(window.location.href).searchParams.has('embedded')) { |
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.
Can we do this when we are building the content for the trace viewer instead in the extension instead?
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.
Unfortunatelly no, because this code must run inside the iframe. But maybe create an embedded.html
/ embedded.tsx
, similar to index.tsx
and uiMode.tsx
, we can just drop this code there as well as instanciating the parameterized WorkbenchLoader
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.
Unfortunatelly no, because this code must run inside the iframe.
I understand that the snapshot renderer should run in an iframe, but the app itself does not have to. Are you doing this for convenience?
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.
The app depends on the SW, and for that reason there's no easy way to render it without the iframe because webviews don't support SW
All bits for embedded mode moved to embedded.tsx. Implemented recommendations from code review.
This comment has been minimized.
This comment has been minimized.
The snapshot popout link is not working ( We can either disable it (it will still work on non-embedded mode) or delegate it to the server (for instance, it can launch a playwright-controlled browser and inject the trace file for sw to load it before opening the |
Actual path to get trace contexts is /contexts, not /context
I found out why I thought I needed to inject the trace file before opening snapshot.html. It's a bug: #31033 |
c0a447d
to
44c101c
Compare
This comment has been minimized.
This comment has been minimized.
Test results for "tests 1"2 failed 27067 passed, 609 skipped Merge workflow run. |
@@ -24,7 +24,8 @@ import { Workbench } from './workbench'; | |||
import { TestServerConnection } from '@testIsomorphic/testServerConnection'; | |||
|
|||
export const WorkbenchLoader: React.FunctionComponent<{ | |||
}> = () => { | |||
embedded?: boolean |
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.
embedded?: boolean | |
supportTraceDrop?: boolean |
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.
embedded
mode only hides the trace viewer header, so maybe call it hideHeader
instead?
Depends on microsoft/playwright#30885 being merged and @playwright/test being updated
Companion PR of microsoft/playwright-vscode#483