feat(compare)!: rebuild Compare & Sync as a native comparison window (#721) - #1968
Conversation
datlechin
added
the
abi-additive
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 20bfb6e978
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let required: PluginCapabilities = mode == .structure ? .schemaCompare : .dataCompare | ||
| guard !driver.capabilities.contains(required) else { return nil } |
There was a problem hiding this comment.
This eligibility check rejects every real database driver because the commit defines the two capability bits but never adds either bit to any PluginDatabaseDriver.capabilities implementation. I checked all driver capability declarations under Plugins; consequently both structure and data comparisons always stop with the unsupported message before doing any work.
Useful? React with 👍 / 👎.
| let result = try await self.executorRun( | ||
| statements: self.statements, | ||
| target: target, |
There was a problem hiding this comment.
When the user changes the editable script preview, the binding only updates editedScript, but Apply still passes the original self.statements here. Thus edits—including removing or correcting a destructive statement—are ignored while the confirmation implies the displayed script will run; either rebuild executable statements from the edited text or make the preview read-only.
Useful? React with 👍 / 👎.
| guard entries.count < limit else { | ||
| truncated = true | ||
| return | ||
| } | ||
| entries.append(entry) |
There was a problem hiding this comment.
For tables exceeding maxRetainedEntries (5,000 by default), this cap discards every later entry even though generateDataScript() builds SQL exclusively from summary.entries. A table whose first 5,000 rows match but whose 5,001st row differs reports the exact difference count yet generates no statement; other large tables are only partially synchronized. Retention used for display must not truncate the data used to build the sync script.
Useful? React with 👍 / 👎.
| if let leftNumber = Double(leftKey), let rightNumber = Double(rightKey) { | ||
| return leftNumber < rightNumber ? .orderedAscending : .orderedDescending |
There was a problem hiding this comment.
When key strings are large exact integers, converting them to Double collapses distinct values—for example, 9007199254740992 and 9007199254740993. The merge join can then choose the wrong stream to advance, misclassify a row present on both sides as an insert, and generate duplicate-key or otherwise incorrect synchronization SQL. Numeric ordering needs an exact decimal/integer comparison consistent with the database ordering.
Useful? React with 👍 / 👎.
| if shouldRollback { | ||
| try? await driver.rollbackTransaction() |
There was a problem hiding this comment.
If rollbackTransaction() fails after cancellation or a statement error, try? silently discards that failure and the returned result still sets rolledBack to true. The apply view then explicitly tells the user that the target is unchanged even though the transaction may remain open or partially applied; propagate the rollback error or record rollback failure separately instead of claiming success.
Useful? React with 👍 / 👎.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
# Conflicts: # CHANGELOG.md # TablePro/Core/Services/Infrastructure/WindowOpener.swift # TablePro/Views/Infrastructure/WindowOpenerBridge.swift
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…ata-path FK ordering Claude-Session: https://claude.ai/code/session_01QZKyY8vPUn3myu82D145pe
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
datlechin
changed the title
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 participant
Closes #721. Replaces this PR's previous contents.
Compare & Sync, rebuilt. The engines and their tests survive; the shell, the I/O layer and the eligibility gate are new, and thirteen correctness defects behind them are fixed.
Why it was rebuilt rather than patched
The feature was inert.
CompareSyncEligibilitygated every comparison onPluginCapabilities.schemaCompare/.dataCompare. A whole-repo grep found those two bits declared inPluginCapabilities.swiftand set only by the test fake inCompareSyncExecutorTests;PluginDatabaseDriver.capabilitiesdefaults to[]. Every Compare press on every real connection returned "does not report structure metadata that can be compared." Nothing here had ever executed against a database, which is why the defects below went unnoticed.The window was the wrong idiom. A comparison is a tool you re-run, not a procedure you walk once, so it was modelled as a four-step wizard. That single choice produced every UI symptom: a wizard has no toolbar, so a numbered step header and a bottom action bar had to be invented, both of which the macOS HIG names directly ("Avoid creating custom window UI"; "Avoid putting critical information or actions in a bottom bar, because people often relocate a window in a way that hides its bottom edge"). Its steps were sequential, so the script got a bare
TextEditorinstead of the app's syntax-highlightedDDLTextView. Its state was transient, so nothing persisted.Apple's own shape for this was measured from the shipped nibs: FileMerge is a chooser plus a persistent window with a customizable, autosaving
NSToolbar. An assistant is for rare irreversible setup.Eligibility
Sixteen SQL drivers now declare the two capability bits. Nine deliberately do not, each for a checked reason: Redis, MongoDB, DynamoDB, etcd, Elasticsearch, Beancount and SurrealDB cannot express
SELECT … ORDER BY …at all; Cassandra's CQL rejectsORDER BYon anything but a clustering column. ClickHouse, BigQuery, Trino and Teradata get.dataCompareonly, because at least one offetchIndexes/fetchForeignKeysis a hardcoded[]and structure compare would under-report. PGlite inherits both from PostgreSQL rather than duplicating the list.CompareCapabilityDeclarationTestsscans the driver sources so this cannot silently regress.Correctness
Every row was verified, several by measurement.
Double, so two BIGINTs above 2^53 paired as one row and the engine emitted an UPDATE that overwrote a different rowDecimal; byte fallback on an unparsable key rather than a shared?? 0sentinelAliceandALICEas one row, a byte comparator does notPluginColumnInfo.collationthreaded intoKeyOrdering, case-folded when the collation is CIDateFormatterclamps a fractional second to milliseconds, sotimestampFractionalDigitsabove 3 was inert and every microsecond difference read as identicalInt64nanoseconds, because scaling aDoubleby 1e9 reintroduced the same precision lossForeignKeyTopologicalSortX'…', which PostgreSQLbytea, MSSQLvarbinaryand OracleRAWall rejectwriteColumns, normalised in the model so no caller can get it wrongfetchTablesreports views, and nothing filtered on kind, so a view reachedSchemaSyncScriptBuilderand yieldedCREATE TABLEPARTITIONED TABLEstill comparessupportsTransactions, so a MySQL structure sync reported a rollback that did not happensupportsTransactionalDDLengineandcollationwere always nil, the notes were dead code andCREATE TABLEdroppedENGINE/CHARSET/COLLATETableDiffResult.comparisonErrorwas read by the UI and written by nothingOn that last one:
SyncStatementcarries hazards computed bySyncSafetyClassifierfrom the typed operation plan. Re-parsing edited text would discard that classification, so read-only is the correct answer rather than the convenient one.Architecture
An endpoint is a
DatabaseScope, not a connection id. Two databases on one server are now a valid pair, and so are two schemas in one database; the read side and the write side finally agree on qualification. Saved comparisons migrate onto scopes rather than being discarded.All I/O moved into
CompareMetadataServiceandCompareRowService, inQueryExecutor's shape, routed throughDatabaseManager.withMetadataDriver(scope:). The old code reachedDatabaseManager.driver(for:)directly, which hands back the connection's single live interactive driver: nometadataRoute, so an embedded engine could be handed a second empty instance, and noSessionDriverGate, so a read could interleave with a tab's query or land on whichever database that tab last switched to. Connections open on demand throughensureConnected, so a comparison no longer requires both windows already open.SessionDriverGateis not reentrant and a session driver holds one database position, so a data comparison of two scopes on one connection is refused up front by name on engines that cannot pool, rather than deadlocking.Snapshot reads run in a bounded
TaskGroup, bounded to 1 forsupportsConnectionPooling == false.Object scope beyond tables. Views, materialized views, procedures, functions and triggers now compare, through the driver methods the sidebar has used since #2383. They have no parsed form, so
SourceObjectDiffEnginecompares normalised definition text andSourceObjectSyncBuilderemits drop-and-create; routines are matched on name and argument list, so two PostgreSQL overloads are not confused.StructureDiffEnginestays table-shaped and keeps its tests;CompareObjectResultunifies both so the results list has one row type.The window
CompareSyncWindowControlleris anNSWindowControllerthat owns its ownNSWindowDelegate(the close guard was a delegate-proxy installed from a backgroundNSViewRepresentable),applyAutosaveNameso the frame is restored and not just recorded, and anNSToolbarwith a stable identifier,allowsUserCustomizationandautosavesConfiguration, attached only once session state exists so a nil-returning delegate cannot poison the saved configuration.Toolbar: Source, Swap, Target, mode, Compare, Group By, Options, Search, Generate Script, Apply. Source and Target are database pickers that walk connection → database → schema, loading each level when its submenu opens. Every item is mirrored under Database > Compare, per the HIG's rule that a toolbar item must also be a menu command; they route by nil target, so they reach the controller only while its window is key.
Body is
AutosavingSplitView. Results are a SwiftUITablewithDisclosureTableRow(both macOS 14.0, confirmed against the SDK interface), so Group By produces real sections. The picker it replaces only re-sorted: both of its cases returned a flat array. Include is a checkbox column with a mixed-state header per group, never aPickerembedded in every row. Detail is Definitions | Rows | Script.Apply opens a resizable sheet with Script / Summary / Warnings; Cancel is the default button, Apply carries
role: .destructive, and Apply stays disabled while any included statement has an unacknowledged hazard. Escape dismisses throughcancelOperation, since Cancel already holds Return.Zero
Color.orange/.green/.redliterals remain underViews/Compare: status tints resolve throughThemeEngine, reusing the data grid's semantic inserted/modified/deleted colours. No hardcoded pixel frame widths.accessibilityDifferentiateWithoutColoris honoured, and accessibility identifiers sit on leaf controls only.A shipped crash, found on the way
SQLExportPluginin 0.67.1 traps onDictionary(uniqueKeysWithValues:)when two tables in different schemas share a name: "Fatal error: Duplicate values for key". Measured underswiftc -O, exit 133. Fixed here, along with two silent siblings the same audit turned up: the export wrote one schema's rows into the other schema's table, and fetched columns and foreign keys for the first export group only, so every later schema silently lost both.PluginExportTablegains aschemafield. The existing initialiser keeps its exact signature and is now@_disfavoredOverload, per the PluginKit ABI rule.PluginKit ABI: additive
scripts/check-pluginkit-abi.shagainst the merge base reports additions only: the newForeignKeyTopologicalSortenum, the two capability bits, andPluginExportTable's new field plus new init overload with the old one preserved. No symbol removed or changed, no@frozenlayout touched. NocurrentPluginKitVersionbump and norelease-all-plugins.shrun. Please add theabi-additivelabel.Registry-only plugins pick up their capability bits on their next release; until then Compare refuses them by name, which is honest.
Verification
verify.sh buildPASS,lintPASS (0 violations acrossTablePro,TableProTests,TableProUITests),docsPASS.verify.sh testover the 29 compare suites: 204 executed, 204 passed.verify.sh abi <merge-base>: additive, as above.verify.sh pluginsfails only on the pre-existingoracle-nio@TaskLocalmacro error in the vendored SPM checkout, which breaks every localAllPluginsbuild and is unrelated to this change. CI compiles plugins on its own runner.New tests pin the defects that shipped: decimal key ordering above 2^53, a case-insensitive collation key, microsecond timestamp inequality and the inert precision setting, a diff past the retention cap reaching the script in full, generated columns never appearing in a write, a VIEW excluded while a PARTITIONED TABLE is not, per-engine binary literals, structure sync refusing a transaction where DDL commits implicitly, two same-named tables in different schemas surviving the topological sort, saved comparisons keyed per database and per schema, and grouping producing real sections.
Review pass
/code-review highover the finished diff found nine defects. All nine are fixed here, each with a test:session.sourceSnapshots, which only the structure path fills, so the ordering degenerated to alphabetical andorder_itemsinserted beforeorders. The data path now records its own snapshots.Accumulatorretained every matched row, so a table with 100,000 matches and ten differences at the end filled the list with matches and dropped every difference, leaving the pane empty under a non-zero count. Only differences are retained now; matches are counted and reported as a number.public.usersandaudit.userscollapsed onto one result and a target-side second schema was never reported. The match key now carries the schema when both sides have one.skippedNullKeyCounthad no reader anywhere, so choosing a nullable key dropped rows from the comparison and the sync while the pane said zero differences. The pane now names the count and what to do about it.endpoint.schemanil, which is what let two schemas' tables collapse, and the generated ALTER carries no schema of its own so it would have landed on whichever schema the connection happened to be on. An endpoint on a schema-capable engine names exactly one schema.("a", "b, c")and("a, b", "c")rendered the same, so excluding one row from a sync excluded the other. Exclusion keys on a separate unambiguous identity; the readable description is unchanged.Not done
docs/images/compare-sync-window*.pngare 1560x960 placeholders so the page renders; they need replacing with real captures before release. There is no "before" shot to pair them with, since the wizard is deleted in this PR.fetchDependentSequences(table:schema:)only, which lists sequences owned by one table rather than a schema-wide catalog. A schema-widefetchSequenceswould be an additive protocol addition; it is the one remaining gap against Navicat's object list.TableProUITests/CompareSyncUITestscovers the menu route, the banner and the disabled states. The flows that need two live fixture connections (an actual run, the close guard firing) are still not automated.https://claude.ai/code/session_01QZKyY8vPUn3myu82D145pe