Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feature(trace-viewer): embedded mode support PoC #30885

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

feature(trace-viewer): embedded mode support PoC #30885

wants to merge 5 commits into from

Conversation

ruifigueira
Copy link
Contributor

@ruifigueira ruifigueira mentioned this pull request May 18, 2024
Open
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@pavelfeldman
Copy link
Member

I'm supportive for those, let me know when they are no longer drafts!

@ruifigueira ruifigueira marked this pull request as ready for review May 21, 2024 19:56
@ruifigueira
Copy link
Contributor Author

ruifigueira commented May 21, 2024 •
edited

@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.

@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@pavelfeldman
Copy link
Member

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.

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?

@ruifigueira
Copy link
Contributor Author

ruifigueira commented May 22, 2024 •
edited

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.

@pavelfeldman
Copy link
Member

we can just rely on the running server instead of the service worker.

Unfortunately, we can't. It is essential for SW to fulfill all the requests.

@ruifigueira
Copy link
Contributor Author

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.

@ruifigueira
Copy link
Contributor Author

Problem explained here, with a workaround proposal:

microsoft/vscode#65452 (comment)

@ruifigueira
Copy link
Contributor Author

That did the trick, key bindings are triggered as expected when trace viewer is focused

@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
pavelfeldman
pavelfeldman reviewed May 24, 2024
View reviewed changes
packages/playwright-core/src/server/trace/viewer/traceViewer.ts Show resolved Hide resolved
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')) {
Copy link
Member

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?

Copy link
Contributor Author

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

Copy link
Member

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?

Copy link
Contributor Author

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

packages/trace-viewer/src/ui/workbenchLoader.tsx Outdated Show resolved Hide resolved
@ruifigueira
chore(trace-viewer): embedded entrypoint added
efd01e7 
All bits for embedded mode moved to embedded.tsx.
Implemented recommendations from code review.
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@ruifigueira
Copy link
Contributor Author

The snapshot popout link is not working (window.open doesn't work in vscode). Using vscode.env.openExternalUrl does not work either because it won't share the sw.

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 snapshot.html URL).

@ruifigueira
fix(trace-viewer): fix snapshot.html
0812599 
Actual path to get trace contexts is /contexts, not /context
@ruifigueira
Copy link
Contributor Author

I found out why I thought I needed to inject the trace file before opening snapshot.html. It's a bug: #31033

@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions
Copy link
Contributor

Test results for "tests 1"

2 failed
❌ [playwright-test] › runner.spec.ts:118:5 › should ignore subprocess creation error because of SIGINT
❌ [installation tests] › playwright-electron-should-work.spec.ts:30:5 › electron should work with special characters in path

27067 passed, 609 skipped
✔️✔️✔️

Merge workflow run.

pavelfeldman
pavelfeldman approved these changes May 28, 2024
View reviewed changes
packages/trace-viewer/src/ui/workbenchLoader.tsx
@@ -24,7 +24,8 @@ import { Workbench } from './workbench';
import { TestServerConnection } from '@testIsomorphic/testServerConnection';

export const WorkbenchLoader: React.FunctionComponent<{
}> = () => {
embedded?: boolean
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
embedded?: boolean
supportTraceDrop?: boolean

Copy link
Contributor Author

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pavelfeldman pavelfeldman pavelfeldman approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@ruifigueira @pavelfeldman