Skip to content

fix: keep edits made during an iCloud sync and SQL file encodings on save, and move diff work off the main thread - #3102

Open
datlechin wants to merge 1 commit into
mainfrom
fix/2505-followups
Open

datlechin wants to merge 1 commit into
mainfrom
fix/2505-followups

Conversation

@datlechin

Copy link
Copy Markdown
Member

Summary

Defects left over from #2505 and the PRs that followed it, in one PR.

Defect Area
An edit made while an iCloud sync was running was reverted by that sync's own echo and never uploaded Sync
After the saved-query store failed to open once, saved queries stayed unavailable until relaunch Storage
Every save of an SQL file wrote UTF-8, dropping the byte order mark, flipping UTF-16/32 byte order and losing encodings macOS records in com.apple.TextEncoding; non-UTF-8 files did not open from Finder or File > Open File… Files
Comparing two query plans ran on the main actor Performance
The Compare & Sync definition diff recomputed on every redraw Performance
A lint error in PersistedTabRoundTripTests Tests

Edits reverted by a sync's own echo

Root cause. The push cleared every pushed record's dirty flag, including a record the user edited again during the round trip. The pull that follows returned this Mac's own pushed copy and applied it over the newer edit, and the next sync found the record clean.

Fix.

  • SyncChangeTracker keeps a monotonic, in-memory edit generation per record. The push snapshots them before it reads any record and clears only the records not edited since.
  • The pull in the same cycle gets a SyncEchoGuard that withholds a record only when it was saved in this cycle and edited since. Withheld records are not cached. Connections are exempt, because their dirty branch already does a three-way merge.
  • A push notification that arrives while a cycle runs is deferred until the cycle ends.
  • A push cut short across batches still reports what it saved (SyncPushInterruption), so the guard and the clearing use the real outcome.
  • The mark APIs are @MainActor, so the process-wide suppression during a pull cannot drop a mark made elsewhere.
  • Tags, groups, SSH profiles and credential profiles mark only records that changed, not the whole collection, so the guard never holds back an untouched sibling.
  • Saved-query writes and remote applies run one at a time through MainActorSerialQueue, so a local edit cannot land between a remote apply's check and its write.
  • Remote deletions discard the deleted records' dirty marks. A mark for a record that no longer exists, or that can never be pushed (a local-only connection), is dropped once its store has been read cleanly. The saved-query store now reports a failed read (readAllFavorites) instead of an empty list.
  • SyncTransport is a seam over CloudKitSyncEngine, so the push, clear and pull sequence runs in tests. The coordinator's push collection, deletion handling and settings code moved to extension files for the SwiftLint limits.

Known limits.

  • The guard lasts for one cycle. If that cycle's pull fails, a later notification pull can still apply the stale echo.
  • Turning sync off and on during a running cycle can start a second one that overlaps it.
  • A genuine change from another device that lands between our push and our pull, on a record we edited meanwhile, is withheld: the local edit wins and goes up on the next push.

Saved-query store stuck after a failed open

SQLFavoriteStorage set its prepared flag before opening. It now keeps a handle only after a successful open and retries on the next access.

SQL file encodings

  • FileTextLoader records how a file was read: the encoding, the byte order mark, and the com.apple.TextEncoding attribute when that attribute alone identified it.
  • FileTextWriter checks that the text fits the encoding. It writes a hidden staging file in the target's folder, with the mark and the bytes in the original byte order, sets the attribute, copies the permissions, then renames it into place. A failure leaves the original untouched, and errors are reported as the matching write failure (no permission, out of space, read-only volume). Symbolic links are written through.
  • Editor saves, Edit Metadata, and Version History Restore and Discard Changes all use it. Git history decodes an older version as UTF-8 first, then in the working copy's recorded encoding.
  • Text the encoding cannot hold is refused with an alert on every save path (Cmd+S, save on close, batch close), and the tab stays open. Save As writes UTF-8, which is how a file is converted. An ASCII-tagged file saves as UTF-8.
  • File > Open File… and opening from Finder read through FileTextLoader. An unreadable file now says so instead of failing silently.
  • The sidebar's warning triangle shows only for encodings that cannot store every character; UTF-16, UTF-32 and GB18030 no longer get it.

Measured on APFS and FAT32: UTF-8, UTF-8 BOM, UTF-16 LE/BE BOM, UTF-32 LE/BE BOM, attribute-only Shift_JIS, windows-1251, macintosh and UTF-16, the Latin-1 fallback, and an empty file all round-trip byte for byte, attribute and file mode included. A read-only tagged file still saves.

Known limit. Keep My Changes in the conflict sheet writes the tab's recorded encoding, even if another app has since converted the file.

Main-thread work

  • QueryPlanComparisonModel.makeContent is @concurrent. Under SWIFT_APPROACHABLE_CONCURRENCY, a plain nonisolated async function runs on its caller's actor.
  • StructureDefinitionDiffView computes its diff once per input in .task(id:), following FileConflictDiffSheet.

Tests

All affected suites pass locally on current main, plus the TableProSyncTests package suites. New suites:

  • Sync: SyncCoordinatorEchoTests, SyncDirtyMarkingTests, SQLFavoriteStorageOpenTests, MainActorSerialQueueTests, PushOutcomeTests.
  • Files: SourceFileEncodingSaveTests, FileTextWriterTests, SQLFileOpeningTests, LinkedSQLFavoriteEncodingTests.
  • Diff: StructureDefinitionDiffPresentationTests.

No UI automation:

  • The encoding flows need files carrying macOS's encoding attribute or unreadable permissions, set up outside the sandboxed UI-test runner, plus modal alerts.
  • The sync flows need CloudKit.

The unit tests drive the same entry points instead.

Deliberately not fixed here

Three changes were written, reviewed and removed, because review kept finding new defects in them:

  • Remote deletions of database favorites and settings are still never applied.
    • Database favorites have fixed ids (a hash of connection and database). Applying the deletion lets this Mac's own delete echo back and erase a favorite the user re-created during the push.
    • Nothing pushes settings tombstones, so the receive path could never fire.
    • A fix needs the deletion echo guarded like saves, and markDirty to cancel a pending tombstone for the same id.
  • A keyword clash still pushes the losing query as a whole record (as fix(sync): stop a pull dropping a saved query whose keyword another query held #3087 shipped).
    • A keyword-only push was tried. Because the record cache is never cleared on account change or iCloud data deletion, it could create a server record holding only the keyword, which another pull then read as a deletion of a live query.
  • Open Quickly's ranking still runs on the main actor. Moving it off broke 24 QuickSwitcherViewModelTests, because results arrived after the tests' flush point.

…save, and move diff work off the main thread
@mintlify

mintlify Bot commented Sep 23, 2026

Copy link
Copy Markdown

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated
TablePro 🟢 Ready View Preview Sep 23, 2026, 8:57 PM

💡 Tip: Enable Automations to automatically generate PRs for you.

This branch was successfully deployed

1 active deployment
staging - docs e0a0b045 Deployed Sep 23, 2026 by mintlify[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant