Fix data loss: saveEngine persisted the favorites list as the clipping history - #13
Open
MiMoHo wants to merge 1 commit into
Open
Fix data loss: saveEngine persisted the favorites list as the clipping history#13MiMoHo wants to merge 1 commit into
MiMoHo wants to merge 1 commit into
Conversation
-saveEngine wrote the current clippingStore under the "jcList" key. While the
favorites store is swapped in, clippingStore IS favoritesStore and the main
store is held in stashedStore, so the favorites list was persisted as the
clipping history and the real history was never written at all.
With savePreference at "after each change" - which is what the preferences
offer and what -actionAfterListModification acts on - this needs no quit: one
'f' in the bezel and one copy is enough, because a copy made while the store is
swapped lands in the favorites store and triggers the save. Quitting does it
too, since -applicationWillTerminate saves. Switching back does not repair it
either: -saveStore:toKey:onDict: clears modifiedSinceLastSaveStore on the store
it wrote, and while swapped that cleared both flags, so the guard then skipped
every later save.
Resolve the main store from wherever it currently is and use it for both the
guard and the "jcList" key. favoritesStore keeps writing "favoritesList", so
the favorites themselves were never affected and still are not.
The guard change is not incidental. On master it tests clippingStore and
favoritesStore, which are the same object while swapped, so a session in which
only the main store changed was skipped entirely and fell back to the last
saved state - a second, quieter way to lose clippings. It now tests the main
store.
Verified with a test program compiled twice, once against this file as it is on
master and once against this version, run against an isolated defaults domain
(history seeded, then 'f', then a copy, then terminate):
master: jcList <- the favorites list, real history never written
9 clippings in, 1 clipping back after the next launch
patched: jcList <- the real history, favoritesList <- the favorites
9 clippings in, 9 back
Also verified: savePreference == 0 is untouched (saveEngine is not called on
that path), and a main store modified before the swap is now saved instead of
being dropped.
Builds with Xcode 26.5, 76 warnings, identical to master.
Co-Authored-By: Claude <noreply@anthropic.com>
Author
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.
The defect
-[FlycutOperator saveEngine]writes the currentclippingStoreunder thejcListkey:While the favorites store is swapped in —
switchToFavoritesStoresetsstashedStore = clippingStore; clippingStore = favoritesStore;— that writes the favorites list as the clipping history, and the real history is never written at all.No quit is needed. With
savePreferenceat "after each change",actionAfterListModificationcallssaveEngineon every list change. Pressfin the bezel, then copy one thing — the copy lands in the swapped-in favorites store and triggers the save. Quitting does it too, viaapplicationWillTerminate.Switching back does not repair it.
saveStore:toKey:onDict:ends withclearModifiedSinceLastSaveStoreon the store it wrote; while swapped that cleared both flags, so the guard then skipped every later save.The favorites themselves were never affected:
favoritesListwas always written fromfavoritesStorecorrectly. The damage is one-directional, which is also how you recognise an affected profile — history and favorites show the same content.The fix
Resolve the main store from wherever it currently is, and use it for both the guard and the
jcListkey.The guard change is not incidental — please keep it. On
masterthe guard testsclippingStoreandfavoritesStore, which are the same object while swapped. A session in which only the main store changed was therefore skipped entirely and fell back to the last saved state: a second, quieter way to lose clippings. It now tests the main store.Verification
A test program compiled twice — once against
FlycutOperator.mas it is onmaster, once against this version — run against an isolated defaults domain. History seeded, thenf, then one copy, then terminate:Two more cases, same harness:
f+ quit with the favorites store never touched: nothing is written on either version, because the guard legitimately skips. So the trigger is the modification while swapped, not the swap itself.savePreference == 0is untouched —saveEngineis not called on that path.Builds with Xcode 26.5 (
BUILD SUCCEEDED), 76 warnings, identical tomasterbuilt the same way. The patch applies without fuzz tomasterand to all ten open branches from this fork.This fix cannot repair an already damaged profile
It works forward only. Once
jcListhas been overwritten with the favorites list, the old history exists nowhere:loadEngineFromPListreadsjcListandfavoritesListand there is no third key, no journal and no backup to reconstruct from. Anyone who has already lost their history needs a file-level backup of~/Library/Preferences/com.generalarcade.flycut.plist(Time Machine), or — coming from the sandboxed build — the older copy under~/Library/Containers/com.generalarcade.flycut/Data/Library/Preferences/, imported by hand, becausecheckAndPerformSandboxDataMigrationbails out as soon as astorekey exists at the new location.An automatic repair heuristic (detect
jcList == favoritesListand clear it) would be guesswork on user data and deliberately is not part of this change.Scope — what is deliberately left alone
This is three effective lines in one method. The audit around it found more in the same family; they are pre-existing, independent, and left for separate PRs:
switchToFavoritesStoreis not re-entrant. Bezel →f→Esc(no path restores the stash) → Preferences → favorites size stepper (checkFavoritesRememberNumPrefswaps unconditionally a second time) →stashedStorebecomes the favorites store and the main store is orphaned. In that state the patched code behaves exactly likemasterand writes the favorites underjcListagain. No regression, but not healed either — the guard belongs with theforPrimaryStore:YESbug at the same call site, in one follow-up.Esc,hideApp,windowDidResignKey:,applicationWillResignActive:. This is what makes all the other findings in this family reachable.Related: #12 fixes the other data-loss path in this area (save-and-delete deleting from the wrong list) and names this change as the separate follow-up it promised. The two are independent — #12 touches only
AppController.m.Not verified
No end-to-end run of the built app, deliberately: a built Flycut writes the real
com.generalarcade.flycutdomain, andHOME=$(mktemp -d)does not isolate CFPreferences on current macOS (NSHomeDirectorystays put), so such a run would have risked real user data. All verification is therefore at harness level —FlycutOperator.mandFlycutEnginecompiled separately, domain = process name. The iCloud/sync paths are dead code here and were assessed by caller analysis, not executed. The re-entrancy trigger above is a code argument; the modal resize dialog was not operated.Authored and reviewed with Claude Code using Claude Opus 5 at
xhighreasoning effort. Cross-checked by a multi-agent adversarial review (independent Opus 5 agents on fix correctness, the persistence and migration paths, same-class defects elsewhere, upstream fit, and specifically on whether any of this is user-visible; every finding then put to three skeptics tasked with refuting it — 38 raised, 18 survived, 20 refuted, none left unchecked). The surviving findings are the scope notes above.🤖 Generated with Claude Code