web: fix save & reset behavior on System ➲ Settings page. #8528
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This page is different from our other pages in that the form is always visible and present and not removed from the DOM at any time during its interaction. Part of the problem was a misapplication of the Lit event cycle, but part of it was just Lit being excessively aggressive with the relationship between values and what is displayed. Primer: An HTML INPUT tag of any kind is either controlled or uncontrolled. An "uncontrolled" INPUT is managed by the browser; a "controlled" tag is managed by the framework, and changes to some state value in the current view dictates both what is shown and when to re-render. For a controlled text input, for example, every change to the input is captured, stored in the state variable, and the input re-rendered to reflect what the state variable says. The confusion here is simple. Our form is usually uncontrolled. When you changed a toggle, the state variable (`this.settings`) *did not change*, so when you hit `[Reset]` Lit saw no reason to re-render the form; the control state hadn't changed at all! A reset form showed the wrong value(s). A call to forcibly `resetForm()` was needed. The other bug, the problem with `[Save]`, was just a race condition. The value was sent to the back-end, then read back, `this.settings` updated, and the form re-rendered. The gap between the re-read and the render created a race condition, and the render usually won, reflecting the *earlier* value. This problem was exacerbated by the implementation of `this.setting`'s getter/setter pair following Lit 3's pattern, when we're currently using Lit 2. Eventing the "update needed," putting an `await()` on the event handler after a save, and implementing the getter/setters correctly, created a better sequences of events: 1. The Form submits and sends a notification that the settings were changed. 2. When the Page receives a notification that the settings have changed, it retrieves the newly-stored settings and stores them locally. 3. When the local settings change, Lit schedules a re-render of the Page. 4. When the Page is re-rendered, the new settings are passed to the Form. 5. When the Form receives new settings, it re-renders the displayed HTML form. You'll note that these are five *separate*, *independent* events. This isn't a sequence. Each of these is a mini-routine that does its job and quits. (2 is most explicit about this: "... a notification...", not "...the notification...") Cleanups, comments, etc, added as needed.
✅ Deploy Preview for authentik-storybook ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for authentik ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
rissson
approved these changes
Feb 15, 2024
|
/cherry-pick version-2024.2 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Details
This page is different from our other pages in that the form is always visible and present and not removed from the DOM at any time during its interaction. Part of the problem was a misapplication of the Lit event cycle, but part of it was just Lit being excessively aggressive with the relationship between values and what is displayed.
Primer: An HTML INPUT tag of any kind is either controlled or uncontrolled. An "uncontrolled" INPUT is managed by the browser; a "controlled" tag is managed by the framework, and changes to some state value in the current view dictates both what is shown and when to re-render. For a controlled text input, for example, every change to the input is captured, stored in the state variable, and the input re-rendered to reflect what the state variable says.
The confusion here is simple. Our form is usually uncontrolled. When you changed a toggle, the state variable (
this.settings) did not change, so when you hit[Reset]Lit saw no reason to re-render the form; the control state hadn't changed at all! A reset form showed the wrong value(s). A call to forciblyresetForm()was needed.The other bug, the problem with
[Save], was just a race condition. The value was sent to the back-end, then read back,this.settingsupdated, and the form re-rendered. The gap between the re-read and the render created a race condition, and the render usually won, reflecting the earlier value. This problem was exacerbated by the implementation ofthis.setting's getter/setter pair following Lit 3's pattern, when we're currently using Lit 2.Eventing the "update needed," putting an
await()on the event handler after a save, and implementing the getter/setters correctly, created a better sequences of events:You'll note that these are five separate, independent events. This isn't a sequence. Each of these is a mini-routine that does its job and quits. (2 is most explicit about this: "... a notification...", not "...the notification...")
Cleanups, comments, etc, added as needed.
Checklist
ak test authentik/)make lint-fix)If an API change has been made
make gen-build)If changes to the frontend have been made
make web)If applicable
make website)