fix(sidebar): rebuild database tree on NSOutlineView to stop the expand/collapse crash#1717
Conversation
…nd/collapse crash
![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: 97ea17b1b8
ℹ️ 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 changed = searchText != view.searchText | ||
| || activeChanged | ||
| || pendingTruncates != view.pendingTruncates | ||
| || pendingDeletes != view.pendingDeletes |
There was a problem hiding this comment.
When databaseFilterSelected changes from one visible subset to another, SwiftUI can re-enter updateNSView, but this changed key ignores the filter set even though rootNodes() uses it. Since refresh() is skipped, the outline keeps showing the previous database list until some unrelated search/active/pending state changes, making the database filter popover appear not to work for partial filters.
Useful? React with 👍 / 👎.
| private func rowActions() -> DatabaseTreeRowActions { | ||
| DatabaseTreeRowActions( | ||
| coordinator: mainCoordinator, | ||
| isReadOnly: mainCoordinator?.safeModeLevel.blocksAllWrites ?? false, |
There was a problem hiding this comment.
When the user switches the connection into read-only/safe mode while the database tree is already visible, existing hosted rows keep the isReadOnly value captured here because the outline is only reloaded for search/active/pending changes. That leaves table context menus in the database tree enabled with the old writeable state, so actions like Truncate/Delete can still be presented and staged until another refresh rebuilds the cells.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 36e3fcbdd4
ℹ️ 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".
| SidebarContextMenu( | ||
| clickedTable: ref.table, | ||
| selectedTables: actions.selectedTables(), | ||
| isReadOnly: actions.isReadOnly, |
There was a problem hiding this comment.
When a table is already selected and the user right-clicks a different, unselected table, this row-level SwiftUI context menu does not update the NSOutlineView selection first. SidebarContextMenu.effectiveTableNames prefers selectedTables whenever it is non-empty, so actions such as Export, Truncate, or Delete are applied to the previous selection while the menu was opened on clickedTable, which can stage operations for the wrong table.
Useful? React with 👍 / 👎.
| let changed = searchText != view.searchText | ||
| || connectionToken != view.connectionToken | ||
| || activeChanged | ||
| || pendingTruncates != view.pendingTruncates | ||
| || pendingDeletes != view.pendingDeletes |
There was a problem hiding this comment.
If SwiftUI reuses this NSViewRepresentable while switching to another connected connection, connectionId and databaseType are assigned before changed is computed and neither value participates in the refresh key. With the same search/active/pending values, the coordinator keeps the old childrenCache/observation dependencies and skips reloadData(), so the sidebar can continue showing the previous connection's database tree until an unrelated state change forces a refresh.
Useful? React with 👍 / 👎.
…ation once per refresh
1 participant
Problem
The connection sidebar database tree crashed on servers with several databases and many schemas (for example PostgreSQL). It crashed on first display and when expanding or collapsing a database. Single-schema databases like SQLite were fine. This reproduced on
main, independent of any open PR.Root cause
The tree was a
List(.sidebar)with manually nestedDisclosureGroups whoseisExpandedwas driven by programmatic bindings (expandedTreeDatabases/expandedTreeDatabaseSchemas), plus a search force-open andexpandActive(). On macOS,DisclosureGroupmisbehaves when opened or closed programmatically with animation (Apple Developer Forums thread 681275). When async schema and table loads re-drove those bindings during an animated outline diff, SwiftUI asserted inViewListTree.visitIteminside theNSOutlineViewcollapse path.OutlineGroupandList(children:)could not replace it: on macOS they are not lazy (FB8782243) and expose no programmatic expansion API, so they would drop lazy loading, auto-expand of the active node, and search reveal.Fix
Rebuild the tree on
NSOutlineViewviaNSViewRepresentable, so the framework owns expansion and the broken programmatic-DisclosureGroup path is gone, while keeping every current behavior.DatabaseTreeOutlineViewwrapsNSScrollView+NSOutlineView(source list, overlay auto-hiding scrollers).DatabaseTreeOutlineCoordinatoris the data source and delegate: lazy load on expand, a loading/empty/error row per node so a driver error never crashes the app, crash-safe programmaticexpandItemfor auto-expand-active and search reveal, and an observation bridge that reconciles whenDatabaseTreeMetadataServicestate changes.DatabaseTreeNodeis the node model with stable ids (cached instances keep expansion and selection across reloads).DatabaseTreeFilteris the extracted, testable filtering.DatabaseTreeRowViewandDatabaseTreeCellViewhost the existing row views and context menus.DatabaseTreeViewkeeps the outer state switch and the search debounce and now embeds the outline. Its public init is unchanged, soSidebarViewis untouched.DatabaseTreeMetadataServiceis unchanged.Both grouping shapes are preserved:
.bySchema(database to schema to objects) and.byDatabase(database to objects). Selection, single-click open, double-click grid focus, multi-select, per-row context menus, active bolding, search filtering, and per-window expansion all carry over.Tests
DatabaseTreeFilterTests: visibility, fuzzy match, content-match reveal, dedup.DatabaseTreeNodeTests: id stability and uniqueness, expandability, table-ref extraction.DatabaseTreeSelectionTestsandSelectionDeltaTestsstay green.DatabaseTreeMetadataServiceTestsmarked@MainActorso the suite builds under Xcode 27 strict concurrency.All pass. Build and lint clean.
Manual verification needed
Build is reviewer-run. Please check on a multi-schema PostgreSQL connection: no crash on load or expand/collapse, lazy load on expand, search reveals matches, single-click opens and double-click focuses the grid, context menus on database/schema/table/routine, and that collapsing the active schema stays collapsed.