fix(datagrid): release table data via direct call instead of broadcast#1139
Merged
Conversation
…on-scoped broadcast
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This was referenced May 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a real bug in the FK-navigate-with-unsaved-changes flow:
Reproducer:
Root cause
MainContentCoordinator.teardown()broadcast a connection-scoped event onAppEvents.shared.mainCoordinatorTeardown. EveryTableViewCoordinatorlistening with.filter { $0.connectionId == myConnectionId }received it — including coordinators in OTHER live windows on the same connection. They ranreleaseData()which empties theirdisplayCache, removes theirtableView.tableColumns, and callsreloadData()on an empty grid.Multi-window same-connection wasn't a concern when the broadcast pattern was added, but the FK-with-changes flow now creates exactly that topology. The filter granularity is too coarse for the actual lifecycle.
What changes
Drop the broadcast pattern entirely. Replace with a direct method call from parent to child:
TablePro/Core/Events/AppEvents.swiftmainCoordinatorTeardownPassthroughSubjectandMainCoordinatorTeardownstructTablePro/Views/Main/MainContentCoordinator.swiftAppEvents.shared.mainCoordinatorTeardown.send(...)withdataTabDelegate?.tableViewCoordinator?.releaseData()TablePro/Views/Results/DataGridCoordinator.swiftobserveTeardown(connectionId:)method, dropteardownCancellable: AnyCancellable?ivar, changeprivate func releaseData()tofunc releaseData()TablePro/Views/Results/DataGridView.swiftobserveTeardowncall sites inmakeNSViewandupdateNSViewThe wiring
MainContentCoordinator -> dataTabDelegate.tableViewCoordinatoralready existed (set duringdataGridAttach), so the direct call path was already in place — just unused.Why this is the Apple-correct pattern
AppKit doesn't use
NotificationCenterorCombinefor parent-to-child cleanup. The documented pattern is direct method dispatch via the ownership graph:NSViewController.viewWillDisappear-> child cleanupNSWindowController.close-> child window releaseNSDocument.close-> dismiss editing UICombine
PassthroughSubjectis for streams of events from indeterminate sources (network, user input, timers). Single-producer / single-consumer parent-child cleanup is exactly the case where direct ownership wins.Sendabletypes and@MainActorboundaries are preserved either way.Diff stats
Test plan