Skip to content

User Pref Editor change cleanup #17025

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
Merged
merged 2 commits into from
Mar 30, 2023
Merged

User Pref Editor change cleanup #17025

merged 2 commits into from
Mar 30, 2023

Conversation

selfcontained
Copy link
Contributor

@selfcontained selfcontained commented Mar 24, 2023 •
edited
Loading

Description

Some small cleanup to the code in user prefs around updating the editor options.

  • Adjusted user update logic to avoid mutating user object in react state directly.
  • Also cleaned up a few places that needed a useCallback() wrapper.

Fixes WEB-39

How to test

  • Go to user preferences
  • Change Editor
  • Click Account in side, then click Preferences
  • Ensure your Editor change is reflected
  • Reload page and ensure it's still reflected.

Release Notes

NONE

Documentation

Build Options:

  • /werft with-werft
    Run the build with werft instead of GHA
  • leeway-no-cache
  • /werft no-test
    Run Leeway with --dont-test
Publish Options
  • /werft publish-to-npm
  • /werft publish-to-jb-marketplace
Installer Options
  • with-ee-license
  • with-dedicated-emulation
  • with-ws-manager-mk2
  • workspace-feature-flags
    Add desired feature flags to the end of the line above, space separated

Preview Environment Options:

  • /werft with-local-preview
    If enabled this will build install/preview
  • /werft with-preview
  • /werft with-large-vm
  • /werft with-gce-vm
    If enabled this will create the environment on GCE infra
  • with-integration-tests=all
    Valid options are all, workspace, webapp, ide, jetbrains, vscode, ssh

@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-bmh-user-prefs-ide-user-context-update.1 because the annotations in the pull request description changed
(with .werft/ from main)

@selfcontained selfcontained changed the title update user in context Update user in context after ide pref change Mar 24, 2023
@selfcontained selfcontained marked this pull request as ready for review March 24, 2023 21:06
@selfcontained selfcontained requested a review from a team March 24, 2023 21:06
@selfcontained selfcontained requested a review from gtsiolis as a code owner March 24, 2023 21:06
@github-actions github-actions bot added the team: webapp Issue belongs to the WebApp team label Mar 24, 2023
@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-bmh-user-prefs-ide-user-context-update.2 because the annotations in the pull request description changed
(with .werft/ from main)

@selfcontained selfcontained changed the title Update user in context after ide pref change User Pref Editor change cleanup Mar 28, 2023
@gtsiolis
Copy link
Contributor

gtsiolis commented Mar 29, 2023 •
edited
Loading

Looking at this now! 👀

UPDATE: Re-spinning the preview environment in https://github.com/gitpod-io/gitpod/actions/runs/4538052905.

@geropl
Copy link
Member

geropl commented Mar 29, 2023

@gtsiolis Triggered again: You have to trigger "all" jobs, otherwise it won't update/install the preview correctly.

@gtsiolis
Copy link
Contributor

gtsiolis commented Mar 29, 2023 •
edited by werft-gitpod-dev-com bot
Loading

@geropl Not sure what happened, but the preview environment is still not deployed. 🤷

@selfcontained selfcontained force-pushed the bmh/user-prefs-ide-user-context-update branch from e4bf847 to 6c3ae2a Compare March 29, 2023 22:22
@selfcontained
Copy link
Contributor Author

I rebased main into it and w/ that latest build it deployed to https://bmh-user-pf016b26807.preview.gitpod-dev.com/workspaces ok. Hopefully it will work for you later still.

gtsiolis
gtsiolis approved these changes Mar 29, 2023
View reviewed changes
Copy link
Contributor

@gtsiolis gtsiolis left a comment

Choose a reason for hiding this comment

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

Thanks, @selfcontained! UX LGTM. 🌮 🌮

Approving to unblock merging but holding in case we'd like someone to take a closer look at the code changes. 🏓

/hold

svenefftinge
svenefftinge approved these changes Mar 30, 2023
View reviewed changes
Copy link
Contributor

@svenefftinge svenefftinge left a comment

Choose a reason for hiding this comment

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

LGTM

components/dashboard/src/user-settings/SelectIDE.tsx Outdated
const newUserData = await updateUser.mutateAsync({ additionalData });
props.updateUserContext && setUser({ ...newUserData });
};
// Avoid mutating user object in state for updates
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is a bit confusing without seeing the previous code. Maybe just remove it?

@selfcontained
Copy link
Contributor Author

/unhold

@roboquat roboquat merged commit 58d8381 into main Mar 30, 2023
@roboquat roboquat deleted the bmh/user-prefs-ide-user-context-update branch March 30, 2023 17:40
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Apr 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gtsiolis gtsiolis gtsiolis approved these changes

@svenefftinge svenefftinge svenefftinge approved these changes

Assignees
No one assigned
Labels
deployed: webapp Meta team change is running in production deployed Change is completely running in production release-note-none size/M team: webapp Issue belongs to the WebApp team
Projects
🍎 WebApp Team
Status: In Validation
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@selfcontained @gtsiolis @geropl @svenefftinge @roboquat