Repository navigation
fix(settings): write pending settings when Settings closes - #2259
Conversation
…osed The spec polled data.json for 3 s after the popout closed. The write waits on a 1 s debounce on the main window's timer, which the OS throttles while that window is hidden, so under load it often landed later (#2213). Assert instead that the change is pending when the popout closes and on disk once the write that closing starts has finished. Refs #2213
A settings change is written by a 1 s debounce on the main window's timer. While a Settings popout covers a hidden main window, the OS throttles that timer, and under load the write landed seconds late, once more than 15 s. Closing Settings now writes the pending change at once. Fixes #2213
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Flush pending saves on close src/main.ts, src/quickAddSettingsTab.ts |
flushPendingSave is now public. QuickAddSettingsTab.hide() invokes it after teardown and mounted-view cleanup, without awaiting it. |
Verify the persisted order tests/e2e/settings-popout-save.test.ts |
The test records whether a save was pending at close, awaits persistChain, and checks the on-disk order against the captured in-memory order. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix · Severity of issue fixed: Medium
Possibly related PRs
- chhoumann/quickadd#2253: It establishes the settings persistence path that this change flushes when the Settings popout closes.
Merge Risk: 
8daa9
-
No actionable merge-blocking risk is established by the supplied evidence.
Pre-merge checks: 
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: flushing pending settings writes when Settings closes. Linked Issues check #2213requires the Settings-popout change to reachdata.jsonafter close and requires an automated test that verifies the resulting state without an unsuitable fixed polling window. `QuickAddSetting…Out of Scope Changes check The changed files are limited to the save method visibility, the Settings tab close path, and the related settings-popout-savee2e test. Each change supports#2213or verifies its required behavior.…Docstring Coverage No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 3… Full details: Linked Issues check
-
Explanation
#2213requires the Settings-popout change to reachdata.jsonafter close and requires an automated test that verifies the resulting state without an unsuitable fixed polling window.QuickAddSettingsTab.hide()now callsflushPendingSave()after view teardown. The method starts the existing queuedpersistSettingspath. The updated e2e test confirms that the save is pending at close, awaitspersistChain, and compares disk order with in-memory order for visible and hidden main windows. The reported load-tested runs also support the fix.
Full details: Out of Scope Changes check
-
Explanation
The changed files are limited to the save method visibility, the Settings tab close path, and the related
settings-popout-savee2e test. Each change supports#2213or verifies its required behavior. No unrelated product behavior is introduced.
Full details: Docstring Coverage
-
Explanation
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 3 files.
-
Finishing Touches
-
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
A rabbit closes Settings with care,
A pending save is sent from there.
The order waits on disk to land,
Checked against the order close at hand.
Then hops away, content and grand.
Comment @coderabbitai help to get the list of available commands.
Fixes #2213.
This changes product behaviour. Do not merge without review.
What changes for users
Closing Settings now writes any pending QuickAdd settings change to
data.jsonat once. Before, the write waited on a 1 s debounce timer on the main window. The behaviour changes most when Settings is a popout and the main window is hidden or minimised. The OS throttles that window's timers, so the write could land seconds late. #2213 measured more than 3 s in 6 of 8 loaded macOS runs, and more than 15 s once. Until the write landed, a sync client or a crash would see the old settings. Nothing else changes. While Settings stays open, writes stay debounced.Change
QuickAddSettingsTab.hide()callsplugin.flushPendingSave(), which is now public. It runs after the Svelte views are torn down, so a global variable edit flushed on unmount (Settings changes made in the Settings popout are not saved when the popout closes within a second #2182) is part of the same write.flushPendingSave()is the same method quit already uses, so the write goes through the same queued, disk-mergingpersistSettingspath.Test
settings-popout-saveused to polldata.jsonfor 3 s. It now asserts two things for both the visible and the hidden main window. The write is still pending when the popout closes, so the spec cannot pass because the debounce already fired. And the change is on disk once the write that closing started has finished. No timing budget is left.Evidence (Linux, isolated instance under a cgroup CPU quota)
pnpm run test(6962 passed) andpnpm run lintpass.Not verified: the macOS run from #2213. On macOS the write still starts at close, so the throttled timer no longer matters. That conclusion is inferred, not measured on macOS. The unexplained 15 s case there could also have been a write that never landed. This fix covers it if the timer was the cause. It does not cover it if the persist chain itself was stuck.
No migration impact.
Note
Medium Risk
Changes when settings hit disk (product behavior) and touches the core
persistSettingspath, though it reuses existing flush-on-exit logic rather than a new persistence mechanism.Overview
Closing QuickAdd Settings now flushes any pending debounced write to
data.jsonimmediately, instead of leaving it on the main window’s 1 s timer (which the OS can throttle when Settings is a popout and the main window is hidden).QuickAddSettingsTab.hide()calls the plugin’sflushPendingSave()after tearing down Svelte views, reusing the same queuedpersistSettingspath as plugin unload/quit.flushPendingSave()is public so the settings tab can invoke it.The settings-popout-save e2e spec now requires a pending save at close, awaits the persist chain after close, and asserts disk matches in-memory order without a multi-second poll—so the test only passes when close triggers the write.
Reviewed by Cursor Bugbot for commit 8daa994. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Flush pending debounced settings saves when Settings closes
QuickAdd.flushPendingSaveis no longer private, andQuickAddSettingsTab.hidecalls it after the settings views are hidden and destroyed. A debounced settings write now also runs when the tab closes, not only on the timer.Macroscope summarized 8daa994.
Summary by CodeRabbit