fix: persist empty connection state after disconnect - #578
Conversation
The persist effect was skipping when openConnectionIds was empty, which meant disconnected connections weren't being cleared from backend storage. This caused the app to reopen connections on next launch even after user had manually closed them. Now we track whether connections have ever been opened (via a ref) and persist all state changes including empty ones after that point. This preserves the original intent of not wiping state during startup before auto-connect runs, while ensuring user-initiated disconnects are properly persisted. Fixes TabularisDB#548
|
Hey @fuleinist, thanks for digging into #548! I took this for a spin locally and found a few things worth discussing before merging. First thing: the new test also passes without the fix 😅 I ran it against Which means the bug in #548 comes from somewhere else. I think I found it:
Repro: open a connection, close it, change any setting (font size, whatever), quit, relaunch. It reconnects. You can watch it happen with There's also a regression risk with the current approach: once So my suggestion would be to fix this in Happy to help test if you want to rework it in that direction! |
Root cause: SettingsProvider was persisting lastOpenConnectionIds on every save_config, overwriting the session state that DatabaseProvider managed. Fix: Strip session-persistence fields before save_config in SettingsProvider. This ensures settings changes don't clobber session state, while keeping DatabaseProvider as the single source of truth for session persistence. - Add stripSessionFields helper in src/utils/settings.ts - Update SettingsProvider.updateSetting() and migration path to strip - Add tests for stripSessionFields Fixes TabularisDB#548
|
Thanks for the thorough review @debba — you're absolutely right on all counts. I've reworked the fix following your suggestion. Instead of adding the ref guard in DatabaseProvider, I'm now stripping the session fields (lastOpenConnectionIds, lastActiveConnectionId) before Changes:
This fixes #548 at the root cause: settings saves can no longer clobber session state, and DatabaseProvider remains the single source of truth for session persistence. The existing DatabaseProvider test that verifies All tests pass (3488/3488), typecheck and lint clean. Pushed as |
|
Thanks @fuleinist for digging into this one and finding the real culprit. The stale session list getting written back on every settings save was sneaky, and stripping the session fields at the source is the right call. Ran the full suite locally and verified the repro from #548 is gone. Merging! |
Summary
Root cause
The persistence effect was skipping when
openConnectionIdswas empty, which meant disconnected connections weren't being cleared from backend storage. This caused the app to reopen connections on next launch even after the user had manually closed them.Fix
Added a ref to track whether connections have ever been opened during the session. Once true, the persistence effect runs for all state changes including empty ones. This preserves the original intent (not wiping state during startup before auto-connect) while ensuring user-initiated disconnects are properly persisted.
Closes #548