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.

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

Make sure the webview is loaded before sending requests #452

Merged
merged 4 commits into from
Dec 24, 2023

Conversation

Or-Geva
Copy link
Contributor

@Or-Geva Or-Geva commented Dec 4, 2023

  • All tests passed. If this feature is not already covered by the tests, I added new tests.
  • I used npm run format for formatting the code before submitting the pull request.

Before this pull request, there was a potential for the Webview to take longer to load than the duration between the IDE sending the first request to the Webview. To address this issue, we now ensure that we wait for a signal indicating the completion of the webview's full loading process before sending any requests.

Dependent on:

@Or-Geva Or-Geva requested a review from yahavi December 4, 2023 15:36
@Or-Geva Or-Geva added bug Something isn't working safe to test Approve running integration tests on a pull request labels Dec 8, 2023
@github-actions github-actions bot removed the safe to test Approve running integration tests on a pull request label Dec 8, 2023
@Or-Geva Or-Geva added the safe to test Approve running integration tests on a pull request label Dec 14, 2023
@github-actions github-actions bot removed the safe to test Approve running integration tests on a pull request label Dec 14, 2023
@Or-Geva Or-Geva requested a review from eyalbe4 December 17, 2023 14:17
Copy link
Member

@yahavi yahavi left a comment

Choose a reason for hiding this comment

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

I'm getting an infinite waiting view after clicking on any component:
image

webview.onDidChangeVisibility(() => {
private async createEventManager(webview: vscode.WebviewView) {
const eventManager: EventManager = await EventManager.createEventManager(webview.webview, this.connectionManager, this._logManager);
webview.onDidChangeVisibility(async () => {
Copy link
Member

Choose a reason for hiding this comment

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

Let's add it to the context subscriptions to allow cleanup on close:

this.context.subscriptions.push(webview.onDidChangeVisibility...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Im not sure im getting your point but we have already performed a cleanup at currentWebview.onDidDispose line 28

Copy link
Member

Choose a reason for hiding this comment

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

I believe that there is a reason that onDidChangeVisibility returns a disposable. Are you sure that currentWebview.onDidDispose disposes of this listener?

src/main/webview/event/eventManager.ts Outdated Show resolved Hide resolved
src/main/webview/event/eventManager.ts Outdated Show resolved Hide resolved
src/main/webview/event/eventManager.ts Outdated Show resolved Hide resolved
@Or-Geva Or-Geva added the safe to test Approve running integration tests on a pull request label Dec 22, 2023
@github-actions github-actions bot removed the safe to test Approve running integration tests on a pull request label Dec 22, 2023
@Or-Geva Or-Geva requested a review from yahavi December 22, 2023 15:04
Copy link
Contributor

👍 Frogbot scanned this pull request and found that it did not add vulnerable dependencies.


Copy link
Member

@yahavi yahavi left a comment

Choose a reason for hiding this comment

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

LGTM

@Or-Geva Or-Geva merged commit 8bdc323 into jfrog:master Dec 24, 2023
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants