Fix wrong model after login (#213) #354
Conversation
🦋 Changeset detectedLatest commit: 2ae86d1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
hassoncs
force-pushed
the
fix-wrong-model-after-login-213
branch
from
4d22dac to
8be91f3
Compare
hassoncs
changed the title
hassoncs
force-pushed
the
fix-wrong-model-after-login-213
branch
from
8be91f3 to
990ece9
Compare
drakonen
left a comment
drakonen
left a comment
There was a problem hiding this comment.
Looks good to me, unsure about the .changeset file if that should be included,
@kevinvandijk should this PR have a .changeset file?
kevinvandijk
left a comment
kevinvandijk
left a comment
There was a problem hiding this comment.
This introduces a regression of a previous bug. Now when changing a setting and then clicking on 'Done', then clicking on 'Discard changes'. It will freeze. See screen shot
I agree. This is a bit of a weird flow but we accepted it for now because usually people will only log in on the welcome flow and logging out here and logging back in is not a common use case. Once we start on customizing it to be our own settingsview we should work on improving that. Eventually we like to add token support as well in #240 but this also requires backend changes. |
hassoncs
force-pushed
the
fix-wrong-model-after-login-213
branch
from
990ece9 to
88b25ae
Compare
|
good catch, @kevinvandijk! It took me a second to figure out, but it seems the bug you found was already on the main branch. I went back to |
kevinvandijk
left a comment
kevinvandijk
left a comment
There was a problem hiding this comment.
Nice find @hassoncs! I was not aware it was in main as well. Seems to work well now here indeed!
Previously we were spreading the entire `prevCachedState.apiConfiguration` object when the keys changed, which would wipe out unsaved changes the user might have made to things like `prevCachedState.apiConfiguration.kiloModel`. Now we only set the keys that might be set via callback and don't trigger a save explicitly. The user will need to click 'save' in order to exit the SettingView like normal (same as changing any other setting). - update state with specific tokens/keys to avoid overwriting unsaved changes - simplify useEffect dependencies by using direct variables **Dev thoughts** This does make me think.. should "login" keys / tokens be a "setting"? seems like it should be in the Settings view UI, but possibly separate from the other settings state of the api. I feel like it's not intuitive to have to click a "save" button once you login to something for it to remain saved. **Test plan**: Verified that logging in / out no longer overwrites other non-saved changes in the Settings. Now users will have to click save for any changes they make, including login state. (This should be ok since we have that confirmation dialog if they don't.) **Closes** #123
…tingsView - remove setTimeout wrapping confirmDialogHandler call for immediate execution I've fixed the bug in the settings view that was causing the "Discard unsaved changes" dialog to appear twice and freeze when changing the diffEnabled setting. The issue was a race condition in the onConfirmDialogResult function. When using setTimeout to delay the callback execution, React would re-render the component before the callback ran. During this re-render, the effect comparing cachedState and extensionState would detect a difference again specifically with the diffEnabled setting, causing isChangeDetected to be set back to true and triggering an infinite loop. The fix removes the setTimeout and calls the callback directly after setting the state, ensuring it runs before any effects can set isChangeDetected back to true. Test plan: - Logged in / out confirmed unsaved settings do not change - update a setting, discard, confirm no freeze - update setting, save, confirm saved
hassoncs
force-pushed
the
fix-wrong-model-after-login-213
branch
from
88b25ae to
2ae86d1
Compare
I've been going down a rabbit hole, trying to figure out why some users were seeing unsaved changes upon rendering the SettingsView for the first time. We've been dealing with this issue multiple times in the past, most recently in #354. I'm of the opinion that at some point we should rework the SettingsView and especially how state is being managed inside of it. There's simply too many opportunity to run into these situations because individual components are in charge of defaults and fallbacks, which will always keep triggering unsaved changes upon first render. With this workaround I simply make sure that we get users out of the state that poisoned their settings state.
…455) * Fix: Workaround for detected changes in SettingsView upon rendering I've been going down a rabbit hole, trying to figure out why some users were seeing unsaved changes upon rendering the SettingsView for the first time. We've been dealing with this issue multiple times in the past, most recently in #354. I'm of the opinion that at some point we should rework the SettingsView and especially how state is being managed inside of it. There's simply too many opportunity to run into these situations because individual components are in charge of defaults and fallbacks, which will always keep triggering unsaved changes upon first render. With this workaround I simply make sure that we get users out of the state that poisoned their settings state. * Don't run in test env because tests are too fast
3 participants
Previously we were spreading the entire
prevCachedState.apiConfigurationobject when the keys changed, which would wipe out unsaved changes the user might have made to things likeprevCachedState.apiConfiguration.kiloModel. Now we only set the keys that might be set via callback and don't trigger a save explicitly. The user will need to click 'save' in order to exit the SettingView like normal (same as changing any other setting).Dev thoughts
This does make me think.. should "login" keys / tokens be a "setting"? seems like it should be in the Settings view UI, but possibly separate from the other settings state of the api. I feel like it's not intuitive to have to click a "save" button once you login to something for it to remain saved.
Test plan: Verified that logging in / out no longer overwrites other non-saved changes in the Settings. Now users will have to click save for any changes they make, including login state. (This should be ok since we have that confirmation dialog if they don't.)