refactor(datagrid): introduce TableRows / Row / Delta value types#930
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8788bc5b8e
ℹ️ 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".
| self.columnDefaults = columnDefaults | ||
| didChange = true | ||
| } | ||
| if let columnForeignKeys, columnForeignKeys != self.columnForeignKeys { |
There was a problem hiding this comment.
Compare foreign keys using stable fields
updateDisplayMetadata currently decides whether foreign keys changed by comparing [String: ForeignKeyInfo] directly, but ForeignKeyInfo has a synthesized equality that includes its id UUID (QueryResult.swift), which is regenerated on each construction. In practice, reloading identical FK metadata from the backend will often look different and return .columnsReplaced, causing unnecessary full metadata invalidations/reloads even when nothing semantically changed.
Useful? React with 👍 / 👎.
Summary
Phase C.1 of the DataGrid refactor. Introduces three new value types —
Row,Delta, andTableRows— alongside the existingRowBuffer/InMemoryRowProvider/RowDataStore. No callers migrated. That's C.2's job. C.1 is small and mergeable so the new shape can be reviewed without the migration churn.Why
Today's row-data system has three tightly coupled types with inconsistent mutation paths (direct buffer writes, inout array refs, provider methods). Sort indices don't auto-invalidate. Inserted rows live in a parallel array. Provider's weak ref to RowBuffer falls through to a shared empty buffer with no error.
Phase C replaces all three with a single value type that emits explicit
Deltaevents on mutation. The eventual controller will applyDeltas directly to NSTableView (insertRows/removeRows/reloadData(forRowIndexes:)), retiring the SwiftUIreloadVersioncounter in Phase D.What
Row+RowIDRowID.insertedreplaces the parallelSet<Int> insertedRowIndices+[Int: [String?]] insertedRowData. UUID is stable across mutations.DeltaTableRowsSendablestruct withContiguousArray<Row>storage. Owns rows + columns + types + metadata (columnDefaults,columnForeignKeys,columnEnumValues,columnNullable). Every mutator returnsDelta:edit(row:column:value:),editMany(_:)appendInsertedRow(values:),appendPage(_:startingAt:)remove(rowIDs:),remove(at:)replace(rows:offset:)updateDisplayMetadata(...)static from(queryRows:columns:columnTypes:...)factoryDeliberate non-features (rationale matters for review):
Equatable— full row equality on big tables is a footgun.DataGridIdentityalready keys onschemaVersion.PendingChanges— composition stays at the coordinator.sortIndices/ display cache /isEvicted— view-state and store concerns, not row-data concerns.pending— caller's job.CellPosition: HashableCellPosition(defined inline atDataGridView.swift:13) was upgraded fromEquatabletoHashableso it can be aSetelement insideDelta.cellsChanged. Single-line change, folded into commit 1 becauseDeltawon't compile without it.Tests
48 cases across three new suites in
TableProTests/Models/Query/:RowTests(8) — RowID factories, isInserted, subscript bounds, equality.DeltaTests(8) — equality across every case,nonesentinel.TableRows*Tests(32 across 9 suites) — construction, reads, edit, editMany, appendInsertedRow (UUID uniqueness, padding, truncation), appendPage, remove (by IDs and IndexSet), replace, metadata, and a regression that mutations preserve column metadata.Files
TablePro/Models/Query/Row.swiftTablePro/Models/Query/Delta.swiftTablePro/Models/Query/TableRows.swiftTablePro/Views/Results/DataGridView.swiftCellPosition: Equatable→HashableTableProTests/Models/Query/RowTests.swiftTableProTests/Models/Query/DeltaTests.swiftTableProTests/Models/Query/TableRowsTests.swiftCHANGELOG.md[Unreleased] / ChangedentryThe existing types —
RowBuffer,InMemoryRowProvider,RowDataStore— are not modified. They coexist with the new types in C.1 and are deleted in C.2.Test plan
swiftlint lint --strict— cleanxcodebuild ... build— succeedsxcodebuild ... test -only-testing:TableProTests/RowTests -only-testing:TableProTests/DeltaTests -only-testing:TableProTests/TableRowsTests— all greenC.2 sketch (next PR, not this one)
Migration order, app green at each step:
TableRowsStore+TableRowsControllerintroducedtableRows.edit(...)returning Deltainout TableRows, returns DeltaRow.idRow.id