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

feat: Display workerName and processInfoId in the console status bar #1173

Merged
merged 7 commits into from
Apr 11, 2023

Conversation

vbabich
Copy link
Collaborator

@vbabich vbabich commented Mar 22, 2023

Get optional workerName and processInfoId along with the login options response from the DHE host, display in the console status bar.

@vbabich vbabich changed the title Display workerName and processInfoId in the console status bar feat: Display workerName and processInfoId in the console status bar Mar 22, 2023
@vbabich vbabich self-assigned this Mar 22, 2023
@vbabich vbabich requested a review from mofojed March 22, 2023 19:58
@codecov
Copy link

codecov bot commented Mar 22, 2023

Codecov Report

Merging #1173 (5307025) into main (5142a4d) will increase coverage by 0.05%.
The diff coverage is 62.74%.

❗ Current head 5307025 differs from pull request most recent head 43a3630. Consider uploading reports for the commit 43a3630 to get more accurate results

@@            Coverage Diff             @@
##             main    #1173      +/-   ##
==========================================
+ Coverage   44.21%   44.26%   +0.05%     
==========================================
  Files         449      450       +1     
  Lines       33449    33480      +31     
  Branches     8409     8420      +11     
==========================================
+ Hits        14789    14820      +31     
  Misses      18610    18610              
  Partials       50       50              
Flag Coverage Δ
unit 44.26% <62.74%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/code-studio/src/main/AppInit.tsx 0.00% <0.00%> (ø)
packages/code-studio/src/main/SessionUtils.ts 0.00% <0.00%> (ø)
...ckages/dashboard-core-plugins/src/redux/actions.ts 27.77% <ø> (ø)
...dashboard-core-plugins/src/panels/ConsolePanel.tsx 46.52% <66.66%> (+0.48%) ⬆️
packages/jsapi-utils/src/MessageUtils.ts 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

packages/code-studio/src/main/AppInit.tsx Outdated Show resolved Hide resolved
packages/dashboard-core-plugins/src/redux/actions.ts Outdated Show resolved Hide resolved
packages/jsapi-utils/src/MessageUtils.ts Outdated Show resolved Hide resolved
Comment on lines 48 to 49
// TODO: checking just the id should be enough, can probably drop the response message check?
if (data?.id !== id || data?.message !== response) {
Copy link
Member

Choose a reason for hiding this comment

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

Yea I think you're right, if we just look at the id, we don't need to bother looking at response, and then the signature can just be requestParentResponse(request, timeout)...

Thinking about this a bit further, there's basically three types of messages we can send:

  • request: Something we expect a response for
  • response: Responding to a request
  • notify: Sending a message we do not expect a response for
    Perhaps we should do that instead... or even just have those as the three different message types... io.deephaven.message.Request, io.deephaven.message.Response, and io.deephaven.message.Notify, and then the payload itself has the details...

I'm just trying to think how we'll want to extend this later for multi-window messaging/embedding in iframes. Let's discuss later today.

mofojed
mofojed previously approved these changes Apr 6, 2023
@vbabich
Copy link
Collaborator Author

vbabich commented Apr 10, 2023

Resolved conflict in ConsolePanel.test.tsx and rebased.

@vbabich vbabich requested a review from mofojed April 10, 2023 19:12
@vbabich vbabich force-pushed the display-worker-info branch from 5307025 to 43a3630 Compare April 10, 2023 20:02
@vbabich vbabich merged commit 85ce600 into deephaven:main Apr 11, 2023
@vbabich vbabich deleted the display-worker-info branch April 11, 2023 15:17
@github-actions github-actions bot locked and limited conversation to collaborators Apr 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants