Skip to content

refactor(datagrid): correctness, a11y, perf, tests#946

Merged
datlechin merged 14 commits intomainfrom
refactor/datagrid-audit-fixes
Apr 29, 2026
Merged

refactor(datagrid): correctness, a11y, perf, tests#946
datlechin merged 14 commits intomainfrom
refactor/datagrid-audit-fixes

Conversation

@datlechin
Copy link
Copy Markdown
Member

Summary

Batch of fixes from the post-#945 datagrid audit. All Tier 1 / Tier 2 / Tier 3 items, plus 48 new tests for previously untested critical paths.

Correctness fixes

  • Cell colors are appearance-aware (DataGridCellView): change-row highlights (deleted/inserted/modified) and the focus ring now re-resolve via viewDidChangeEffectiveAppearance() when the user toggles Light/Dark mid-session.
  • Native focus ring (DataGridCellView): replaced the hardcoded CALayer border with focusRingMaskBounds + drawFocusRingMask() so the ring tracks the system accent color, contrast settings, and appearance automatically.
  • Overlay editor discards on column resize (CellOverlayEditor): a column resize while editing a multiline cell now discards the in-progress edit instead of silently committing partial text.
  • sortedIDs / cachedRowCount sync (DataGridView + coordinator): the SwiftUI bridge that writes new sortedIDs to the coordinator now also calls updateCache() immediately, removing the window where numberOfRows could disagree with the live sort permutation.

Accessibility

  • VoiceOver sort state announcements (SortableHeaderCell): column headers now announce sort direction and multi-sort priority (e.g. "Column: Name, Sorted ascending, Priority 2") via accessibilityLabel() override.

Performance

  • Display formats memoized (MainContentCoordinator): ValueDisplayDetector.detect now runs once per result schema instead of on every SwiftUI body evaluation. Cache keyed by (tab.id, schemaVersion, smartDetectionEnabled, overridesVersion).
  • Dropped redundant acceptsMouseMovedEvents (SortableHeaderView): the window-level flag was redundant with the tracking area; removed.

Test coverage (+48 tests)

  • FileColumnLayoutPersisterTests: clear-removes-file, isolation across connections, save-overwrites, columnOrder == nil round-trip, empty JSON recovery (+7).
  • ColumnIdentitySchemaTests: positional schema doesn't resolve raw duplicates, name-based schema doesn't resolve col_N, reserved __rowNumber__ triggers positional, three-duplicates path (+7).
  • MainContentCoordinatorTabSwitchTests: 19 tests covering filter/visibility/changeManager/selection/toolbar save+restore, switch-from-nil, switch-to-nil, unknown ids, isHandlingTabSwitch lifecycle.
  • MainContentCoordinatorSortTests: 15 tests covering single/multi-column cycles, removal, replacement across columns, schemaVersion cache invariants, pagination reset.

Out of scope (future work)

  • MainContentCoordinator → services split (1,502-line god class). Larger refactor, separate PR.
  • Hidden columns dual persistence (FileColumnLayoutPersister widths/order vs ColumnVisibilityManager hidden state). Needs design decision on canonical store.
  • Sort completion ordering (flagged by sort/cache agent): async sort completion fires dataGridDidReplaceAllRows() before SwiftUI pushes new sortedIDs, so same-row-count re-sorts may render in old order until next mutation. Separate fix.
  • Sort race "stale completion" guard test: requires >1000 rows + careful task scheduling to be non-flaky.

Test plan

  • Toggle Light → Dark mode while a cell with deleted/inserted/modified highlight is visible: highlight color updates immediately, no need to scroll.
  • Set system accent to graphite/red, focus a cell with keyboard: focus ring renders in the accent color, not blue.
  • Open multiline overlay editor on a cell, type partial text, double-click a column divider: edit is discarded, cell value unchanged.
  • Sort a column then sort another: no NSTableView assertion crash on rapid sort changes.
  • Sort a 100-column wide table; scroll/select repeatedly: no UI jank from displayFormats re-detection.
  • VoiceOver: navigate to a sorted column header, hear "Column: <name>, Sorted ascending" (or descending, with priority for multi-sort secondary).
  • xcodebuild test — all four new test files pass.
  • swiftlint lint --strict — 0 violations.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6a7c4523ff

ℹ️ 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".

Comment on lines +544 to +548
if let cached = coordinator.displayFormatsCache[tab.id],
cached.schemaVersion == tab.schemaVersion,
cached.smartDetectionEnabled == smartDetectionEnabled,
cached.overridesVersion == overridesVersion {
return cached.formats
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Invalidate display format cache on result-set switch

The cache lookup only keys on tab.id, schemaVersion, settings, and overridesVersion, so it returns cached formats even after switchActiveResultSet(to:in:) replaces tableRows for the same tab without bumping schemaVersion (see MainContentCoordinator+TableRowsMutation.swift). In pinned multi-result workflows, switching between result sets with different columns/sample values can reuse stale detected formats, causing wrong per-column rendering (or missing formatting) until another schema-version-changing action occurs.

Useful? React with 👍 / 👎.

@datlechin datlechin changed the title refactor(datagrid): audit batch — correctness, a11y, perf, tests refactor(datagrid): correctness, a11y, perf, tests Apr 29, 2026
@datlechin datlechin merged commit a176782 into main Apr 29, 2026
2 checks passed
@datlechin datlechin deleted the refactor/datagrid-audit-fixes branch April 29, 2026 20:01
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