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

Update 'noExperiments' context after trace open #297

Conversation

williamsyang-work
Copy link
Contributor

@williamsyang-work williamsyang-work commented Jan 15, 2025

What it does

Fixes #296.
Triggers an update to the 'trace-explorer.noExperiments' context after a trace or experiment is opened.

How to test

The steps to reproduce the bug can be found in #296.
The bug can be reproduced on master
The bug should be fixed on this branch.

Follow-ups

When the server is online, the context is properly updated by a vscode-message sent from the frontend recieved by the backend.

Frontend:

// 105-112 vscode-trace-explorer-opened-traces-widget.tsx
private initialized = false;
protected doHandleOpenedTracesChanged(payload: OpenedTracesUpdatedSignalPayload): void {
    if (!this.initialized) {
        this.initialized = true;
        return;
    }
    this._signalHandler.updateOpenedTraces(payload.getNumberOfOpenedTraces());
}

Backend:

// 116-118 trace-explorer-opened-traces-webview-provider.ts
case VSCODE_MESSAGES.OPENED_TRACES_UPDATED:
    updateNoExperimentsContext();
    return;

I'm not sure why this code is triggered when the server is online and not when the server is offline. I didn't dig too deep into investigating that because I'm not sure the relevancy, and I know that the events getting bounced around on the FE / BE can get really messy and hard to trace.

There is also the tracking of initialized and we may simply not be sending the first signal intentionally. This can be investigated because it may not be necessary to track anymore and could lead a cleaner solution.

I didn't deep dive into this. I think updating the context on the backend after a trace is opened is a reasonable approach. WDYT?

Review checklist

  • [x ] As an author, I have thoroughly tested my changes and carefully followed the instructions in this template

Triggers an update to the 'trace-explorer.noExperiments' context after
a trace or experiment is opened.

Signed-off-by: Will Yang <[email protected]>
@williamsyang-work williamsyang-work force-pushed the update-experiments-context-with-trace-open branch from cc0e635 to 438b2a7 Compare January 15, 2025 19:10
@williamsyang-work williamsyang-work marked this pull request as ready for review January 15, 2025 19:11
return;
progress.report({ message: ProgressMessages.COMPLETE, increment: 30 });
} finally {
updateNoExperimentsContext();
Copy link
Contributor

Choose a reason for hiding this comment

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

That's a creative way of ensuring to update that context after that "leaky" block executes, without adding it in a bunch of places - I like

Copy link
Contributor

@marcdumais-work marcdumais-work left a comment

Choose a reason for hiding this comment

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

LGTM - thanks for this contribution!

@marcdumais-work
Copy link
Contributor

I think updating the context on the backend after a trace is opened is a reasonable approach. WDYT?

seems reasonable to me

Copy link
Collaborator

@bhufmann bhufmann left a comment

Choose a reason for hiding this comment

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

Looks good to me. This change fixed it. Thanks!

@bhufmann bhufmann merged commit 47e7087 into eclipse-cdt-cloud:master Jan 17, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants