Skip to content

refactor: replace AnyChangeManager closure erasure with ChangeManaging protocol#915

Merged
datlechin merged 2 commits into
mainfrom
refactor/change-managing-protocol
Apr 27, 2026
Merged

refactor: replace AnyChangeManager closure erasure with ChangeManaging protocol#915
datlechin merged 2 commits into
mainfrom
refactor/change-managing-protocol

Conversation

@datlechin
Copy link
Copy Markdown
Member

Summary

  • AnyChangeManager was a closure-based type erasure class with 7 @ObservationIgnored closure properties and changes: [Any] forcing as? RowChange runtime downcasts
  • Now uses a ChangeManaging protocol that both DataChangeManager and StructureChangeManager conform to
  • AnyChangeManager is a thin @Observable wrapper with a single wrapped: any ChangeManaging property
  • changes: [Any] replaced with typed rowChanges: [RowChange]
  • All as? RowChange downcasts eliminated (2 call sites in DataGridCoordinator + DataGridView)
  • Net -34 lines (71 added, 105 removed)

Test plan

  • Edit cells in data tab — verify changes tracked and displayed (colored rows)
  • Undo/redo edits — verify works
  • Save changes — verify SQL generated correctly
  • Structure tab — verify column/index/FK changes tracked
  • Structure tab undo/redo — verify works
  • Create table — verify change manager works
  • Switch between data and structure tabs — verify no cross-contamination

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: d9f1b7752f

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

wrapped.consumeChangedRowIndices()
}

init(_ manager: any ChangeManaging) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Update test-facing AnyChangeManager API changes

This refactor removes the labeled initializers in favor of init(_:) (and also renames the exposed change list to rowChanges), but the existing test target still calls AnyChangeManager(dataManager:), AnyChangeManager(structureManager:), and wrapper.changes in TableProTests/Core/ChangeTracking/AnyChangeManagerTests.swift. In environments that build tests (CI/local test runs), this change set will fail to compile until those call sites are updated or compatibility shims are kept.

Useful? React with 👍 / 👎.

@datlechin datlechin merged commit 7e4f856 into main Apr 27, 2026
2 checks passed
@datlechin datlechin deleted the refactor/change-managing-protocol branch April 27, 2026 15: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