-
Notifications
You must be signed in to change notification settings - Fork 3
feat: Core - Sync homeserver settings #633
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
Conversation
a70ce21 to
a2ccb36
Compare
MiguelMedeiros
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issues & Recommendations
🔴 Critical: Race Condition in Controller
Location: src/core/controllers/settings/settings.ts (lines 17-22)
private static async commitUpdate(): Promise<void> {
const pubky = Core.useAuthStore.getState().selectCurrentUserPubky();
const settings = Core.SettingsNormalizer.extractState(Core.useSettingsStore.getState());
await Core.SettingsApplication.commitUpdate(settings, pubky);
}If multiple settings changes happen rapidly, each call extracts state independently. Earlier in-flight requests may overwrite later local changes when they complete, causing data loss.
Suggestion: Consider debouncing or using a queue pattern:
private static pendingCommit: Promise<void> | null = null;
private static async commitUpdate(): Promise<void> {
// Wait for any pending commit to complete before starting a new one
if (this.pendingCommit) {
await this.pendingCommit;
}
const pubky = Core.useAuthStore.getState().selectCurrentUserPubky();
const settings = Core.SettingsNormalizer.extractState(Core.useSettingsStore.getState());
this.pendingCommit = Core.SettingsApplication.commitUpdate(settings, pubky);
await this.pendingCommit;
this.pendingCommit = null;
}🟡 Medium: Muted Users Syncing Creates Unnecessary Network Calls
Location: src/core/controllers/settings/settings.ts (lines 87-104)
The muted array is explicitly not synced (normalizer always returns empty array from homeserver). However, the controller has methods like addMutedUser, removeMutedUser, setMutedUsers, clearMutedUsers that all call commitUpdate().
This triggers homeserver sync for changes that won't actually persist remotely.
Suggestion: Either:
- Remove these methods from the controller (keep only direct store access), OR
- Add a
skipSyncparameter, OR - Include
mutedin the homeserver sync
@afterburn can you address those so we can merge this PR?
|
@MiguelMedeiros resolved the issues you flagged |
Initial setup for homeserver settings syncing, lmk if you think I need to take it into a different direction @tipogi