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

Detect if dashboard has been accessed due to workspace idle redirect + workspace idle error #556

Merged
merged 1 commit into from
Jun 22, 2022

Conversation

dkwon17
Copy link
Contributor

@dkwon17 dkwon17 commented Jun 9, 2022

Signed-off-by: David Kwon dakwon@redhat.com

What does this PR do?

eclipse-che/che-operator#1392 reroutes a workspace's mainUrl (ex. https://<che-host>/workspacee3dab47b6f6441c6/theia-ide/3100/#/)
to the dashboard.

This PR reads and stores the window.location.pathname from the dashboard frontend preload script to determine whether the dashboard is being accessed via workspace idle redirection. The window.location.pathname is stored in the browser's sessionStorage.

To check whether the workspace idle error page is displayed, the PR checks the sessionStorage for the initial window.location.pathname and checks if there is a devworkspace with the same mainUrl in the redux store. If yes, the error page is displayed.

Error page provided by this PR:
image

Here is a demo:

demo.mov

What issues does this PR fix or reference?

Part of eclipse-che/che#20599

Is it tested? How?

For testing, please see #556 (comment)

Release Notes

Docs PR

@openshift-ci
Copy link

openshift-ci bot commented Jun 9, 2022

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@che-bot
Copy link
Contributor

che-bot commented Jun 9, 2022

Click here to review and test in web IDE: Contribute

@che-bot
Copy link
Contributor

che-bot commented Jun 9, 2022

Click here to review and test in web IDE: Contribute

@codecov
Copy link

codecov bot commented Jun 9, 2022

Codecov Report

Merging #556 (fe00e0f) into main (3f21a14) will decrease coverage by 0.09%.
The diff coverage is 35.13%.

@@            Coverage Diff             @@
##             main     #556      +/-   ##
==========================================
- Coverage   56.22%   56.12%   -0.10%     
==========================================
  Files         217      217              
  Lines        7374     7410      +36     
  Branches     1264     1270       +6     
==========================================
+ Hits         4146     4159      +13     
- Misses       3049     3071      +22     
- Partials      179      180       +1     
Flag Coverage Δ
unittests 56.12% <35.13%> (-0.10%) ⬇️

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

Impacted Files Coverage Δ
...dashboard-frontend/src/services/bootstrap/index.ts 0.00% <0.00%> (ø)
...-frontend/src/services/bootstrap/issuesReporter.ts 28.57% <0.00%> (ø)
...ard-frontend/src/services/session-storage/index.ts 66.66% <25.00%> (-33.34%) ⬇️
...-frontend/src/Layout/ErrorReporter/Issue/index.tsx 79.31% <70.00%> (-4.91%) ⬇️
packages/dashboard-frontend/src/preload/index.ts 83.33% <100.00%> (+2.25%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3f21a14...fe00e0f. Read the comment docs.

@github-actions
Copy link

github-actions bot commented Jun 9, 2022

Docker image build succeeded: quay.io/eclipse/che-dashboard:pr-556

@dkwon17
Copy link
Contributor Author

dkwon17 commented Jun 9, 2022

This feature would need to be throughly tested. While trying to record the demo video, I ran into a situation where the 'workspace stopped' error page appears when it shouldn't. The bug can be seen at about 0:33s of this video:

workspace-idled-bugged.mov

There is a The workspace status changed unexpectedly to "Starting" error (I'm not sure what caused that error) when starting the workspace. At this point, we shouldn't be redirected to the 'workspace stopped' error page.

@github-actions
Copy link

github-actions bot commented Jun 9, 2022

Docker image build succeeded: quay.io/eclipse/che-dashboard:pr-556

@dkwon17
Copy link
Contributor Author

dkwon17 commented Jun 14, 2022

With the latest commit, the links in the error page have been implemented
image

The first link directs the user to /ide/:namespace/:workspaceName which is used to restart the stopped workspace.
The second link directs the user to /workspace/:namespace/:workspaceName which is the workspace details page.

First link demo:
https://user-images.githubusercontent.com/83611742/173587485-4dfa5f0c-5b64-40b0-b463-f01bb7eb65c6.mov

Second link demo:
https://user-images.githubusercontent.com/83611742/173588244-d6ec9dcb-7e96-43dd-b1f5-987f3284bc50.mov

@dkwon17 dkwon17 marked this pull request as ready for review June 14, 2022 13:27
@che-bot
Copy link
Contributor

che-bot commented Jun 14, 2022

Click here to review and test in web IDE: Contribute

@che-bot
Copy link
Contributor

che-bot commented Jun 14, 2022

Click here to review and test in web IDE: Contribute

@dkwon17 dkwon17 changed the title WIP: Detect if dashboard has been accessed due to workspace idle redirect + workspace idle error Detect if dashboard has been accessed due to workspace idle redirect + workspace idle error Jun 14, 2022
type: IssueType;
error: Error;
data?: T;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @olexii4 @akurinnoy , how does adding data here this look? I added this to pass extra links for when rendering the error page:

case 'workspaceStopped':
return this.renderWorkspaceStoppedError(
issue.error,
(issue as Issue<WorkspaceRoutes>).data,
);

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that we are going to extend this part very often
As for me, I prefer to use just WorkspaceRoutes until we will need something else. I don't think we will extend this part in the near future.

@akurinnoy WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As for me, I prefer to use just WorkspaceRoutes until we will need something else.

Do you mean add a private field for WorkspaceRoutes inside of IssuesReporterService?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the suggestion, in my latest push, I've changed:

data?: T;

to

data?: WorkspaceRoutes

@github-actions
Copy link

Docker image build succeeded: quay.io/eclipse/che-dashboard:pr-556

1 similar comment
@github-actions
Copy link

Docker image build succeeded: quay.io/eclipse/che-dashboard:pr-556

@che-bot
Copy link
Contributor

che-bot commented Jun 14, 2022

Click here to review and test in web IDE: Contribute

@github-actions
Copy link

Docker image build succeeded: quay.io/eclipse/che-dashboard:pr-556

@github-actions
Copy link

Docker image build succeeded: quay.io/eclipse/che-dashboard:pr-556

Copy link
Contributor

@olexii4 olexii4 left a comment

Choose a reason for hiding this comment

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

Well done!
Tested. It works as expected.

@che-bot
Copy link
Contributor

che-bot commented Jun 20, 2022

Click here to review and test in web IDE: Contribute

@che-bot
Copy link
Contributor

che-bot commented Jun 20, 2022

Click here to review and test in web IDE: Contribute

@github-actions
Copy link

Docker image build succeeded: quay.io/eclipse/che-dashboard:pr-556

@dkwon17
Copy link
Contributor Author

dkwon17 commented Jun 20, 2022

/retest

@openshift-ci openshift-ci bot added the lgtm label Jun 21, 2022
@olexii4
Copy link
Contributor

olexii4 commented Jun 21, 2022

https://youtu.be/wdTb64-r34Q

Comment on lines 216 to 228
here
</a>
to restart your workspace.
</p>
<p
className=""
data-pf-content={true}
>
Click
<a
onClick={[Function]}
>
here
Copy link
Member

Choose a reason for hiding this comment

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

here is UX antipattern [1] that we should try to avoid, so let's probably have just restart and return links

[1] https://issues.redhat.com/browse/CRW-3024

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, my latest push updates the links:
image

Copy link
Member

@ibuziuk ibuziuk left a comment

Choose a reason for hiding this comment

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

Great improvement 👍
@dkwon17 just please remove here links before merging

Signed-off-by: David Kwon <dakwon@redhat.com>
@che-bot
Copy link
Contributor

che-bot commented Jun 21, 2022

Click here to review and test in web IDE: Contribute

@openshift-ci
Copy link

openshift-ci bot commented Jun 21, 2022

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: akurinnoy, dkwon17, ibuziuk, olexii4

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@github-actions
Copy link

Docker image build succeeded: quay.io/eclipse/che-dashboard:pr-556

@ibuziuk ibuziuk merged commit f6562c6 into main Jun 22, 2022
@ibuziuk ibuziuk deleted the workspace-idle-page branch June 22, 2022 12:10
@che-bot che-bot added this to the 7.50 milestone Jun 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants