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(server): Add pull session status websocket handler (#2016) #2145

Merged
merged 12 commits into from
Dec 6, 2022

Conversation

andre-code
Copy link
Contributor

@andre-code andre-code commented Nov 16, 2022

PR to add session status websocket handler.

  • (ui) Always check if there are changes in the session status using the websocket channel.
  • (ui) Ignore changes if the pathname is a sessionUrl (*/sessions,*/sessions/new, */sessions/show/* ).
  • (ui) if there is a change and the pathname is not a session page pull the session using the NotebooksCoordinator.
  • (server-ui) Add websocket handler that check the sessions status object and send a message ("true") when there is a change.
  • Add Tests

Closes #2016

/deploy #persist

@andre-code andre-code temporarily deployed to renku-ci-ui-2145 November 16, 2022 16:16 Inactive
@RenkuBot
Copy link
Contributor

You can access the deployment of this PR at https://renku-ci-ui-2145.dev.renku.ch

@andre-code andre-code changed the title feat(server): Add pull session status websocket handler (#2016) WIP feat(server): Add pull session status websocket handler (#2016) Nov 16, 2022
@andre-code andre-code temporarily deployed to renku-ci-ui-2145 November 16, 2022 16:38 Inactive
@andre-code andre-code force-pushed the 2016-ui-server-session-pooling-websocket branch from 679b8a4 to 28a941f Compare November 23, 2022 16:36
@andre-code andre-code temporarily deployed to renku-ci-ui-2145 November 23, 2022 16:36 Inactive
@andre-code andre-code force-pushed the 2016-ui-server-session-pooling-websocket branch from 28a941f to 5253850 Compare November 23, 2022 16:58
@andre-code andre-code temporarily deployed to renku-ci-ui-2145 November 23, 2022 16:58 Inactive
@andre-code andre-code force-pushed the 2016-ui-server-session-pooling-websocket branch from 5253850 to 6b46bb5 Compare November 26, 2022 18:54
@andre-code andre-code temporarily deployed to renku-ci-ui-2145 November 26, 2022 18:54 Inactive
@andre-code andre-code temporarily deployed to renku-ci-ui-2145 November 26, 2022 19:23 Inactive
@andre-code andre-code force-pushed the 2016-ui-server-session-pooling-websocket branch from 6b46bb5 to 64d55fe Compare November 26, 2022 22:10
@andre-code andre-code changed the title WIP feat(server): Add pull session status websocket handler (#2016) feat(server): Add pull session status websocket handler (#2016) Nov 26, 2022
@andre-code andre-code marked this pull request as ready for review November 26, 2022 22:10
@andre-code andre-code requested a review from a team as a code owner November 26, 2022 22:10
@andre-code andre-code temporarily deployed to renku-ci-ui-2145 November 26, 2022 22:10 Inactive
@andre-code andre-code force-pushed the 2016-ui-server-session-pooling-websocket branch from d742a0c to 3be4639 Compare November 29, 2022 16:53
@andre-code andre-code force-pushed the 2016-ui-server-session-pooling-websocket branch from 3be4639 to 80df1ee Compare November 29, 2022 16:54
@andre-code andre-code temporarily deployed to renku-ci-ui-2145 November 29, 2022 17:02 Inactive
@andre-code andre-code temporarily deployed to renku-ci-ui-2145 November 30, 2022 10:50 Inactive
Copy link
Member

@lorenzo-cavazzi lorenzo-cavazzi left a comment

Choose a reason for hiding this comment

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

The PR looks good! I have just a couple of changes, mainly to:

  • prevent handling the auth headers in the low-level fetch functions
  • store the minimum amount of data necessary when using channel.data.set

@andre-code andre-code temporarily deployed to renku-ci-ui-2145 December 1, 2022 15:36 Inactive
@andre-code andre-code temporarily deployed to renku-ci-ui-2145 December 1, 2022 15:53 Inactive
@andre-code andre-code temporarily deployed to renku-ci-ui-2145 December 1, 2022 16:12 Inactive
Copy link
Member

@lorenzo-cavazzi lorenzo-cavazzi left a comment

Choose a reason for hiding this comment

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

Great, almost there! I have just another detail on how we handle server-side headers so that we can keep that coherent with the changes already done in the kg-search branch.

@andre-code andre-code temporarily deployed to renku-ci-ui-2145 December 2, 2022 09:59 Inactive
@andre-code andre-code temporarily deployed to renku-ci-ui-2145 December 2, 2022 10:47 Inactive
@andre-code andre-code temporarily deployed to renku-ci-ui-2145 December 2, 2022 15:57 Inactive
@andre-code andre-code temporarily deployed to renku-ci-ui-2145 December 5, 2022 09:40 Inactive
@andre-code andre-code temporarily deployed to renku-ci-ui-2145 December 5, 2022 10:50 Inactive
Copy link
Member

@lorenzo-cavazzi lorenzo-cavazzi left a comment

Choose a reason for hiding this comment

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

Great! This works well!
I tested a bit more with anonymous users and I submitted a change to clean the headers (it should work but give it a quick test) along with a couple of other cosmetic changes. Nothing big, the PR is basically ready.

Also, give a run to npm run lint on the "server" folder. There are a couple of lining errors but somehow the tests are not failing on that
image

@andre-code andre-code force-pushed the 2016-ui-server-session-pooling-websocket branch from 0d17a4c to 848d6ae Compare December 5, 2022 18:46
@andre-code andre-code temporarily deployed to renku-ci-ui-2145 December 5, 2022 18:46 Inactive
client: add case for url when project is called sessions
@andre-code andre-code force-pushed the 2016-ui-server-session-pooling-websocket branch from 848d6ae to 20de8ee Compare December 5, 2022 19:11
@andre-code andre-code temporarily deployed to renku-ci-ui-2145 December 5, 2022 19:11 Inactive
@andre-code andre-code temporarily deployed to renku-ci-ui-2145 December 5, 2022 19:26 Inactive
@andre-code andre-code temporarily deployed to renku-ci-ui-2145 December 6, 2022 08:30 Inactive
@andre-code andre-code temporarily deployed to renku-ci-ui-2145 December 6, 2022 09:12 Inactive
Copy link
Member

@lorenzo-cavazzi lorenzo-cavazzi left a comment

Choose a reason for hiding this comment

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

This works well! Let's merge it 🚀

@andre-code andre-code deployed to renku-ci-ui-2145 December 6, 2022 15:50 Active
@andre-code andre-code merged commit 1427fd7 into master Dec 6, 2022
@andre-code andre-code deleted the 2016-ui-server-session-pooling-websocket branch December 6, 2022 17:15
@RenkuBot
Copy link
Contributor

RenkuBot commented Dec 6, 2022

Tearing down the temporary RenkuLab deplyoment for this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move the sessions polling to the server and notify using the websocket channel
3 participants