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.

Sign up for GitHub

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

Jump to bottom

Redirect web page when instance changed #9004

Merged
merged 1 commit into from
Apr 6, 2022
Merged

Redirect web page when instance changed #9004

merged 1 commit into from
Apr 6, 2022

Conversation

mustard-mh
Copy link
Contributor

@mustard-mh mustard-mh commented Mar 29, 2022 •
edited
Loading

Description

Display opening workspace when ide is not ready yet. Relates PR #8125

Refresh web page when instanceId changed to get ide config and ws works again

Need to verify

  1. One frame running phase does not display (will display Starting, Opening workspace...
  2. Stopped workspace URL contains not_found works after ws started
  3. Stopped workspace works after ws started Redirect web page when instance changed #9004 (comment)

Related Issue(s)

Fixes #8990
Fixes #8226

How to test

  1. Start same workspace in tab 1\2\3 (tab1 <-> verify1/3, tab2 <-> verify2
  2. After they started, stop workspace in dashboard
  3. Wait until tabs 1\2\3 Stopped
  4. Refresh tab2
  5. Go to tab3 click Open Workspace
  6. See if tabs 1\2\3 work fine

We can do negative test like this PR #8125 in staging here https://hw-fix-8990-crash.staging.gitpod-dev.com/workspaces which will make Insiders VSCode crash bb705e2

Release Notes

Fixed one frame running phase when starting workspace
Fixed IDE options display incorrectly when restarting the workspace
Fixed stopped workspace does not work again if it started in another place

Documentation

@mustard-mh mustard-mh requested a review from a team March 29, 2022 16:28
@github-actions github-actions bot added the team: webapp Issue belongs to the WebApp team label Mar 29, 2022
@mustard-mh mustard-mh requested review from akosyakov and geropl March 30, 2022 06:28
geropl
geropl previously requested changes Mar 30, 2022
View reviewed changes
Copy link
Member

@geropl geropl left a comment

Choose a reason for hiding this comment

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

We introduced this change to avoid endless loops where we would start a new workspace on each page load.

IMO the problem here is somewhere else.

@mustard-mh
Copy link
Contributor Author

We introduced this change to avoid endless loops where we would start a new workspace on each page load.

IMO the problem here is somewhere else.

@geropl Code this PR changed, only effect display, logic not changed

@geropl
Copy link
Member

geropl commented Mar 30, 2022

@mustard-mh Sorry for not being more precise here (still struggling with the virous 😵 ).

We need the buttons to be there in case of a restart. Also cmp. my later comment here.

@roboquat roboquat added size/XS and removed size/M labels Mar 30, 2022
@mustard-mh mustard-mh marked this pull request as draft March 30, 2022 09:10
@mustard-mh mustard-mh marked this pull request as ready for review March 30, 2022 15:33
@akosyakov akosyakov requested a review from geropl March 30, 2022 15:40
@akosyakov
Copy link
Member

akosyakov commented Mar 31, 2022 •
edited
Loading

Eh, now we have a problem in another place:

  • tab 1: start any workspace
  • tab 2: open the same workspace
  • note how both end up in the IDE
  • stop the workspace
  • note how both end up in "Stopped"
  • open the workspace again from tab 2
  • Note how:
    • both workspaces show up-to-date state
    • only tab 2 reveals the IDE
    • tab 1 still shows the up-to-date state and controls to open/reveal the IDE (with fresh credentials), but it just hangs in opening workspace... forever

@mustard-mh could you add this to how to test

@geropl
Copy link
Member

geropl commented Mar 31, 2022

tab 1 still shows the up-to-date state and controls to open/reveal the IDE (with fresh credentials), but it just hangs in opening workspace... forever

@akosyakov Ah, yes. That's why I originally combined this with #8226 . If we combine this with #8978 I think we should be good! 🙃

@akosyakov
Copy link
Member

ok, let's try, @mustard-mh Could you bring other changes?

@mustard-mh mustard-mh marked this pull request as draft April 1, 2022 14:01
@mustard-mh mustard-mh marked this pull request as ready for review April 3, 2022 20:52
@mustard-mh mustard-mh requested a review from a team April 3, 2022 20:52
@mustard-mh mustard-mh marked this pull request as draft April 3, 2022 20:54
@roboquat roboquat added size/M and removed size/S labels Apr 3, 2022
@mustard-mh mustard-mh changed the title [dashboard] display opening workspace when ide is not ready yet Redirect web page when instance changed Apr 3, 2022
@mustard-mh mustard-mh marked this pull request as ready for review April 4, 2022 07:45
@mustard-mh
Copy link
Contributor Author

Also updated how to test section

@mustard-mh
Copy link
Contributor Author

mustard-mh commented Apr 6, 2022 •
edited by werft-gitpod-dev-com bot
Loading

@geropl Please have a look 🙏

andreafalzetti
andreafalzetti approved these changes Apr 6, 2022
View reviewed changes
Copy link
Contributor

@andreafalzetti andreafalzetti left a comment

Choose a reason for hiding this comment

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

I've tested all scenarios and it works as expected.

Great work @mustard-mh 🚀

geropl
geropl reviewed Apr 6, 2022
View reviewed changes
components/supervisor/frontend/src/index.ts
if (currentInstanceId !== "") {
if (instance.id !== currentInstanceId && instance.ideUrl !== "") {
currentInstanceId = instance.id;
window.location.href = instance.ideUrl;
Copy link
Member

Choose a reason for hiding this comment

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

💯

geropl
geropl approved these changes Apr 6, 2022
View reviewed changes
Copy link
Member

@geropl geropl left a comment

Choose a reason for hiding this comment

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

LGTM - thx for fixing this! 💪

@roboquat roboquat merged commit 43a41f2 into main Apr 6, 2022
@roboquat roboquat deleted the hw/fix-8990 branch April 6, 2022 17:01
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed: IDE IDE change is running in production deployed Change is completely running in production labels Apr 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akosyakov akosyakov akosyakov left review comments

@geropl geropl geropl approved these changes

@andreafalzetti andreafalzetti andreafalzetti approved these changes

Assignees
No one assigned
Labels
deployed: IDE IDE change is running in production deployed: webapp Meta team change is running in production deployed Change is completely running in production release-note size/M team: IDE team: webapp Issue belongs to the WebApp team
Projects
None yet
Milestone
No milestone
5 participants
@mustard-mh @geropl @akosyakov @andreafalzetti @roboquat