Skip to content

Conversation

@afterburn
Copy link
Contributor

@afterburn afterburn commented Dec 18, 2025

Initial setup for homeserver settings syncing, lmk if you think I need to take it into a different direction @tipogi

@afterburn afterburn force-pushed the feat/homeserver-settings branch from a70ce21 to a2ccb36 Compare December 18, 2025 16:36
@afterburn afterburn marked this pull request as ready for review January 5, 2026 13:45
@afterburn afterburn requested a review from tipogi January 5, 2026 13:46
@afterburn afterburn self-assigned this Jan 5, 2026
@afterburn afterburn added this to the El Salvador milestone Jan 6, 2026
@afterburn afterburn added 📈 enhancement New feature or request ⚙️ core labels Jan 6, 2026
Copy link
Member

@MiguelMedeiros MiguelMedeiros left a 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:

  1. Remove these methods from the controller (keep only direct store access), OR
  2. Add a skipSync parameter, OR
  3. Include muted in the homeserver sync

@afterburn can you address those so we can merge this PR?

@afterburn
Copy link
Contributor Author

@MiguelMedeiros resolved the issues you flagged

@MiguelMedeiros MiguelMedeiros merged commit 90fc458 into master Jan 8, 2026
3 checks passed
@MiguelMedeiros MiguelMedeiros deleted the feat/homeserver-settings branch January 8, 2026 03:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

⚙️ core 📈 enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants