fix(editor): make Format SQL undoable and sort query results server-side without overwriting the editor (#1645)#1658
Conversation
…ide without overwriting the editor (#1645)
![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: e5953f369a
ℹ️ 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".
| if tab.tabType == .query { | ||
| if !newState.columns.isEmpty && tab.pagination.hasMoreRows { | ||
| let baseQuery = tab.pagination.baseQueryForMore ?? tab.content.query | ||
| let strippedQuery = Self.stripTrailingOrderBy(from: baseQuery) | ||
| let orderClause = newState.columns.compactMap { sortCol -> String? in | ||
| guard sortCol.columnIndex >= 0, sortCol.columnIndex < tableRows.columns.count else { return nil } | ||
| let columnName = tableRows.columns[sortCol.columnIndex] | ||
| let direction = sortCol.direction == .ascending ? "ASC" : "DESC" | ||
| return "\(queryBuilder.quoteIdentifier(columnName)) \(direction)" | ||
| }.joined(separator: ", ") | ||
| let orderQuery = orderClause.isEmpty ? strippedQuery : "\(strippedQuery) ORDER BY \(orderClause)" | ||
| tabManager.mutate(at: tabIndex) { tab in | ||
| tab.sortState = newState | ||
| tab.hasUserInteraction = true | ||
| tab.pagination.resetLoadMore() | ||
| tab.content.query = orderQuery | ||
| } | ||
| runQuery() | ||
| return | ||
| } | ||
|
|
||
| if newState.columns.isEmpty { | ||
| tabManager.mutate(at: tabIndex) { tab in | ||
| tab.sortState = newState | ||
| tab.hasUserInteraction = true | ||
| } | ||
| querySortCache.removeValue(forKey: tab.id) | ||
| dataTabDelegate?.dataGridDidReplaceAllRows() | ||
| return | ||
| } | ||
|
|
||
| let baseQuery = tab.pagination.baseQueryForMore ?? tab.content.query | ||
| let strippedQuery = Self.stripTrailingOrderBy(from: baseQuery) |
There was a problem hiding this comment.
When a query tab result comes from Run All Statements or a selection containing multiple statements, executeMultipleStatements does not populate tab.pagination.baseQueryForMore for the active result, so this new query-tab sorting path falls back to the full editor text. For an editor containing e.g. SELECT ...; SELECT ..., clicking a result-grid header builds one script with ORDER BY appended at the end instead of sorting the displayed statement, which commonly fails or targets the wrong statement; the removed client-side sort did not have this regression. The sort source needs to be stored per result set or otherwise use the exact statement that produced the active result.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91a02f1d04
ℹ️ 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".
| return "\(queryBuilder.quoteIdentifier(columnName)) \(direction)" | ||
| return "\(self.queryBuilder.quoteIdentifier(columnName)) \(direction)" | ||
| }.joined(separator: ", ") | ||
| let orderQuery = orderClause.isEmpty ? strippedQuery : "\(strippedQuery) ORDER BY \(orderClause)" |
There was a problem hiding this comment.
When a query-tab result comes from SQL that already has LIMIT or OFFSET, appending the new clause here produces invalid clause order such as SELECT ... LIMIT 100 ORDER BY "name" ASC. The table-tab builder already inserts ORDER BY before LIMIT/OFFSET, but this new server-side query-tab path bypasses that handling, so clicking a grid header fails for common limited queries.
Useful? React with 👍 / 👎.
| if tab.tabType == .table { | ||
| sql = fullQuery | ||
| if let sortOverride = tab.pagination.sortExecutionOverride { | ||
| tabManager.mutate(at: index) { $0.pagination.sortExecutionOverride = nil } |
There was a problem hiding this comment.
If the sorted query contains parameters and the parameter panel is currently hidden, runQuery() clears sortExecutionOverride before the parameter-panel visibility check later returns without dispatching the query. In that scenario, sorting a parameterized result only opens the panel, then the next run uses tab.content.query without the generated ORDER BY, while the tab's sort state still indicates the grid should be sorted.
Useful? React with 👍 / 👎.
1 participant
Fixes #1645 — two undo-related bugs in the SQL file editor.
Bugs
ORDER BYand wrote it back into the editor'scontent.query; for an opened.sqlfile that made it dirty, so Cmd+S persisted the generated query to disk, and the overwrite couldn't be undone.Both shared one root cause: writing
tab.content.queryprogrammatically routed through the editor'ssetText->setTextStorage->CEUndoManager.clearStack(), which wipes the undo stack.Bug 1: Format is now a single undoable edit
Format applies through the editor's undoable API instead of the SwiftUI text binding.
SQLEditorCoordinator.performFormatSQL()formats the text view's contents and applies them withTextView.replaceCharacters(in:with:), which registers oneCEUndoManagermutation — so one Cmd+Z restores the pre-format text, matching every editor users know. All entry points (toolbar button, app menu, context menu) route throughEditorEventRouter(the existing find-panel pattern); the deadonFormatSQLplumbing is removed.Bug 2: Sorting a query result runs server-side and never touches the editor
Query-result sorting is now server-side end-to-end:
ORDER BYagainst the database (via a new execution-onlyPaginationState.sortExecutionOverride), so the editor text and any backing.sqlfile are never rewritten. Clearing the sort re-runs the statement withoutORDER BY.This removes the old client-side in-memory sort path for query results (
querySortCache,QuerySortCacheEntry,multiColumnSortedIDs,activeSortTasks,sortedIDsForTab, and the cache clears inRowEditingCoordinator). The.tablebrowse view is unchanged (its query is app-generated, not user text).Tests
content.queryunchanged and the file not dirty; clearing sort preserves the editor query;PaginationState.resetLoadMore()clears the override.NSApp.keyWindow, so it isn't unit-testable headlessly; the formatter logic itself is already covered.Notes
ORDER BYto the executed statement after stripping a trailing one — the codebase's existing mechanism, shared with load-more. A statement ending inLIMITwould need sub-select wrapping to be fully correct; happy to add that if wanted.RowSortComparatoris now unused (it backed the removed in-memory sort); left in place as a self-contained tested utility, easy follow-up to remove.