Repository navigation
Fix settings state: fire event before async storage to ensure React updates - #10
Merged
Merged
Conversation
…pdates Co-Authored-By: Danial Piterson <danial.samiei@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
This file contains hidden or 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
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.
Fix settings state: fire change event before async storage persistence
Summary
Reorders
_onDidChangeState.fire()to execute beforeawait _storeState()across all 7 setter methods inVoidSettingsService. Also wraps_storeState()internals in a try-catch so storage failures don't prevent the change event from firing.Problem: In the web deployment (ide.orcest.ai), entering the OpenRouter API key in the onboarding wizard has no effect — the UI still says "Please set up at least one Chat model." The in-memory state updates correctly (
this.state = _validatedModelState(newState)is synchronous), but React components only re-render when_onDidChangeState.fire()is called. Sincefire()was placed afterawait _storeState(), if_storeState()throws (e.g. due to web-mode IndexedDB/encryption issues), the event never fires and the UI never updates.Fix: Move
fire()before theawait, and wrap_storeState()in try-catch so storage failures are logged but don't block the UI.Review & Testing Checklist for Human
_storeState()is actually throwing — this is inferred from the symptom. Check browser console after deploying for[VoidSettingsService] Failed to store state:messages to confirm. If no such messages appear, the real root cause is elsewhere and this PR won't fix the onboarding issue.fire()succeeds even if storage fails — which is better for UX but could mask data loss.fire()beforeawait _storeState(), rapid successive calls (e.g. typing quickly in the API key input) could cause overlapping_storeState()calls. Verify no state corruption occurs in storage.Notes
_onUpdate_syncApplyToChat()(removed space before{) — not intentional but harmless.this-binding issues.Link to Devin run: https://app.devin.ai/sessions/9a3910ef8208402b93e14fa38bb90f2b
Requested by: @danialsamiei