-
Notifications
You must be signed in to change notification settings - Fork 53
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
Small fixes #181
Small fixes #181
Conversation
Code coverage diff between base branch:master and head branch: CHE-19070
|
✅ E2E dashboard-next tests succeed 🎉 See Details
Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)
|
Docker image build succeeded: docker.io/maxura/che-server:che-dashboard-pull-181 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found another tiny bug which would be good to fix but not critical, \n are not displayed correctly:
{"jsonrpc":"2.0","method":"runtime/log","params":{"time":"2021-03-05T09:37:41.809521613Z","text":"List of plugins and editors to install\n- redhat/java/latest - Java Linting, Intellisense, formatting, refactoring, Maven/Gradle support and more...\n- eclipse/che-machine-exec-plugin/nightly - Che Plug-in with che-machine-exec service to provide creation terminal or tasks for Eclipse Che workspace containers.\n- eclipse/che-theia/next - Eclipse Theia, get the latest release each day.\n","runtimeId":{"ownerId":"a2f45de2-006b-4b46-a3eb-66db9f286f72","envName":"default","workspaceId":"workspace37y0tb6hifcdrdk3"}}}
❌ E2E dashboard-next tests failed ❗ See Details
Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)
|
Code coverage diff between base branch:master and head branch: CHE-19070
|
Docker image build succeeded: docker.io/maxura/che-server:che-dashboard-pull-181 |
❌ E2E dashboard-next tests failed ❗ See Details
Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)
|
Code coverage diff between base branch:master and head branch: CHE-19070
|
Docker image build succeeded: docker.io/maxura/che-server:che-dashboard-pull-181 |
❌ E2E dashboard-next tests failed ❗ See Details
Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)
|
Code coverage diff between base branch:master and head branch: CHE-19070
|
Docker image build succeeded: docker.io/maxura/che-server:che-dashboard-pull-181 |
❌ E2E dashboard-next tests failed ❗ See Details
Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)
|
Code coverage diff between base branch:master and head branch: CHE-19070
|
Docker image build succeeded: docker.io/maxura/che-server:che-dashboard-pull-181 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue with duplicated logs entries is fixed after eclipse-che/che-workspace-client#60
But this PR does a lot of (probably the same in master) redundant subscribe/unSubscribe to JSON RPC which I would like to see dropped in the scope of this PR.
❌ E2E dashboard-next tests failed ❗ See Details
Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)
|
src/containers/IdeLoader.tsx
Outdated
public async componentWillUnmount(): Promise<void> { | ||
this.debounce.unsubscribeAll(); | ||
window.removeEventListener( | ||
'message', | ||
event => this.handleMessage(event) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure it'll work as expected cause the callback in this line is not the same callback object as added here: https://github.com/eclipse/che-dashboard/pull/181/files#diff-9d36030402d9b9e891ad48f6ffe964d9e83fc5962a3b907a5d0d1928f44776eeR178
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. I have fixed it.
src/store/Workspaces/index.ts
Outdated
@@ -408,7 +439,7 @@ const mapMerge = (originMap: Map<string, string[]>, additionalMap: Map<string, s | |||
const res = new Map<string, string[]>(); | |||
originMap.forEach((val: string[], key: string) => { | |||
const merge = (val: string[], newVal: string[] | undefined): string[] => { | |||
if (!newVal || (val.length > 0 && newVal.length === 1 && val[val.length - 1] === newVal[0])) { | |||
if (!newVal || (val.length > 0 && newVal.length === 1)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we definitely need a unit test for this function
…Output channel Signed-off-by: Oleksii Orel <[email protected]>
Signed-off-by: Oleksii Orel <[email protected]>
Signed-off-by: Oleksii Orel <[email protected]>
Signed-off-by: Oleksii Orel <[email protected]>
Signed-off-by: Oleksii Orel <[email protected]>
✅ E2E dashboard-next tests succeed 🎉 See Details
Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)
|
Code coverage diff between base branch:master and head branch: CHE-19070
|
Docker image build succeeded: docker.io/maxura/che-server:che-dashboard-pull-181 |
@olexii4 I faced a bug, not sure it's introduced by this PR though steps to reproduce:
Screen.Recording.2021-03-10.at.11.25.06.mov |
❌ E2E dashboard-next tests failed ❗ See Details
Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)
|
@olexii4 after the last commit I can't reproduce this issue. |
Signed-off-by: Oleksii Orel <[email protected]>
❌ E2E dashboard-next tests failed ❗ See Details
Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)
|
No changes to code coverage between the base branch and the head branch |
What does this PR do?
This PR provides several improvements:
What issues does this PR fix or reference?
fixes eclipse-che/che#19070
fixes eclipse-che/che#19174
fixes eclipse-che/che#18917
fixes eclipse-che/che#18589
fixes eclipse-che/che#19234
Signed-off-by: Oleksii Orel [email protected]