refactor(datagrid): persistent column pool + typed cell hierarchy#951
refactor(datagrid): persistent column pool + typed cell hierarchy#951
Conversation
…te reorder flicker
datlechin
changed the title
![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: 36c04cfb35
ℹ️ 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 columnsChanged = !latestRows.columns.isEmpty | ||
| && coordinator.lastReconciledColumnNames != latestRows.columns | ||
|
|
||
| let isInitialDataLoad = structureChanged && oldRowCount == 0 && !latestRows.columns.isEmpty | ||
| let shouldRebuildColumns = columnsChanged || isInitialDataLoad |
There was a problem hiding this comment.
This rebuild gate only checks schema/name changes, so updates to configuration.hiddenColumns do not trigger reconcileColumnPool. In flows like dataGridHideColumn/dataGridShowAllColumns, the column list stays the same, shouldRebuildColumns remains false, and the else branch only updates isEditable, leaving NSTableColumn.isHidden stale until some unrelated schema change happens.
Useful? React with 👍 / 👎.
| for slot in 0..<pooledColumns.count where slot >= visibleCount && !attachedIdentifiers.contains(pooledColumns[slot].identifier) { | ||
| tableView.addTableColumn(pooledColumns[slot]) |
There was a problem hiding this comment.
Adding every pooled slot to tableView.tableColumns (even slots >= visibleCount) introduces hidden phantom columns after switching from a wider table to a narrower one. Because NSTableView.numberOfColumns includes hidden columns, keyboard navigation logic that advances by column index can move focus into these non-schema columns, causing edit/focus behavior to target columns that viewFor later rejects as out of range.
Useful? React with 👍 / 👎.
… layout-skip optimization
1 participant
Summary
DataGrid Phase 2: replaces the destructive
rebuildColumnsand 15-paramDataGridCellFactory.makeDataCellmega-cell with:DataGridColumnPoolkeeping stableNSTableColumninstances so NSTableView's cell reuse pool stays warm across table switchesDataGridBaseCellView+ 7 typed leaf subclasses (text, foreignKey, dropdown, boolean, date, json, blob) each with their own reuse identifierDataGridCellRegistryresolving kind from column type/FK/dropdown flags and dequeuing the typed cellColumnIdentitySchema(dataColumn-N) with name to slot translation APIsNSAnimationContext+CATransaction.disableActionswrap onreconcileto commit rename and reorder in a single visual frameWhy
CPU spiked to 100% on every table click. Instrumentation traced 87% of
viewFortime to per-cell allocation (~0.26 ms/cell). Root cause:rebuildColumnsremoved and recreated allNSTableColumninstances on every table switch, which discarded all cell views from NSTableView's reuse pool. Every visible cell had to allocateDataGridCellView+CellTextField+ 2NSButtons + 2 SF Symbol lookups + 12NSLayoutConstraints from scratch.Results
viewFor batchfor 14 cols x 1000 rows: ~135 ms → 41 mssectionA makeView/reuseper cell: 0.26 ms → 0.00 ms (reuse hit)Audit issues addressed
#5 (partial), #7 (prepared, native widgets defer to Phase 2c), #19, #20, #21, #28, #40, #50. See
docs/development/datagrid-audit.md.Out of scope (deferred)
Design doc
docs/development/datagrid-phase2-design.mdTest plan
xcodebuild test -only-testing:TableProTests/DataGridColumnPoolTestsxcodebuild test -only-testing:TableProTests/DataGridCellRegistryTestsxcodebuild test -only-testing:TableProTests/ColumnIdentitySchemaTestsswiftlint lint --strict