fix(datagrid): carry every per-table setting through a rename and a connection delete - #2746
Merged
Merged
Conversation
This was referenced Sep 11, 2026
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.
Summary
Five stores keep settings keyed by connection, database, schema and table. Each one had to be wired by hand into three lifecycle events: a table rename, a database or schema rename, and a connection delete. Two were wired into none of them.
FilterSettingsStorageFileColumnLayoutPersisterHighlightRuleStorage.unreadable.jsoncopy stayedValueDisplayFormatStorage(Display As)ForeignKeyLabelColumnStoreSo a Display As format or a foreign key label was lost on any rename and outlived the connection it belonged to. A deleted connection's column layouts stayed on disk and in iCloud.
Fix
TableScopedSettingsStoreprotocol (renameTable,renameContainer,purgeConnections) andTableScopedSettingsRegistry.stores, which lists all five shared instances.movePerTableSettings,retargetContainerandConnectionLocalState.purgeloop over the registry, so a new store is wired into all three events by being registered.renameLastFilters/renameScope/removeFilters(for:),rename/renameScopeon the layout persister, andrename/renameScope/removeRuleson highlight rules. Every caller and test is updated.UserDefaultsstores conform through newKeyValueStore.keys(withPrefix:)plusmoveValue/moveValues/removeValueshelpers, andTableScope.storagePrefix(connectionId:). A table rename moves one exact key, soorders_archiveis untouched whenordersis renamed. The container prefix ends in a separator, soshoppingis untouched whenshopis renamed.SyncChangeTracker.markDeleted(_:ids:), backed bySyncMetadataStorage.removeDirty(_ ids:)andaddTombstones. It does one read-modify-write and posts one change notification, where 300 saved layouts would otherwise post 300 notifications and rewrite a growing tombstone list 300 times. On a delete that arrived from iCloud, the tracker is already suppressed insideapplyRemoteChanges, so no tombstones are pushed back.Highlight rules (#2738) are on main as
8a9ed73a3, so they conform here. The purge fix for their unreadable file needs no CHANGELOG line because the feature is unreleased.Import and export are not affected:
ConnectionExportEnvelopecarries connections, groups, tags and credentials only.Known gap, not fixed here: foreign key labels on MySQL and MariaDB
The foreign key picker keys its label through
ForeignKeyLookupService.tableScope(from:reference:), which putsreferencedSchemain the schema slot. On MySQL that value isREFERENCED_TABLE_SCHEMA, which is the database name, so a label is saved under(db: shop, schema: shop, table: users). A table rename builds(db: shop, schema: nil, table: users), the same scope every other per-table store uses, so the label key is not found and the label is still lost. A database rename rewrites the prefix but leavesschema: shop, so the picker then looks understore.store.The fix is to make the label key the referenced table's own scope, which needs a rule for engines whose "schema" is a database (and for cross-database references), plus a migration for labels already saved. That is a change to the picker's key, not to the lifecycle wiring, so it is left for a follow-up. PostgreSQL, SQL Server, Oracle and other engines with a real schema layer are fixed by this PR.
Tests
ValueDisplayFormatStorageTests,ForeignKeyLabelColumnStoreTests: table, schema and database rename, purge, and the longer-name traps (orders_archive,public_old,shopping,chinook_backup). Display As also covers legacy key migration on rename and legacy key removal on purge.FilterSettingsStorageTests: a table rename moves filters and browse search, read back from a fresh instance. A schema rename moves browse search.ColumnLayoutSyncTests: purge removes the file, leaves other connections alone, and tombstones exactly the purged records. A rename tombstones the old record and marks the new one dirty.HighlightRuleStorageTests: purge removes the unreadable file.SyncChangeTrackerTests: a batchmarkDeletedclears each dirty flag and tombstones each id once, an empty batch does nothing, and suppression makes it a no-op.TableScopedSettingsRegistryTests: purge reaches every store once with every id, an empty purge reaches none, and a source scan ofTablePro/Core/Storagefails if a class keyed byTableScope,CompositeStorageKeyorColumnLayoutTableKeydoes not conform or is not registered.No UI test: the behaviour is storage bookkeeping, covered at the store and registry level.
Build passes. The nine affected suites run 102 cases, all passing:
ValueDisplayFormatStorageTests,ForeignKeyLabelColumnStoreTests,FilterSettingsStorageTests,HighlightRuleStorageTests,ColumnLayoutSyncTests,FileColumnLayoutPersisterTests,TableScopedSettingsRegistryTests,TableScopeTestsandSyncChangeTrackerTests. SwiftLint strict is clean over all 20 changed files, the test target andPackages/included.