fix: resolve reported data grid, editor, and filter UX issues#1773
fix: resolve reported data grid, editor, and filter UX issues#1773datlechin wants to merge 6 commits into
Conversation
![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: 6ed9dc8f87
ℹ️ 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".
| onDismiss: { tab.display.activeResultSet?.errorMessage = nil } | ||
| onDismiss: { | ||
| tab.display.activeResultSet?.errorMessage = nil | ||
| tab.execution.errorMessage = nil |
There was a problem hiding this comment.
When the banner is showing tab.execution.errorMessage, this dismiss handler tries to assign through the QueryTab value captured by resultsSection(tab:). Unlike activeResultSet (a reference type), execution is a stored struct property on an immutable function parameter, so this breaks the build instead of clearing the error; route the dismissal through tabManager.mutate(tabId:) or another mutable tab-state path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73bd3840c3
ℹ️ 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".
| } | ||
|
|
||
| func fixErrorWithAI(error: String) { | ||
| guard let query = lastQueryErrorSQL else { return } |
There was a problem hiding this comment.
This reads a coordinator-global lastQueryErrorSQL, which is overwritten by the next failing query in any tab. Because the banner is stored per tab via tab.execution.errorMessage, a user can fail a query in tab A, fail a different query in tab B, then return to tab A and click Fix with AI; the AI receives tab B's SQL paired with tab A's error. Store or pass the failed SQL with the tab/result that owns the banner instead of using a single coordinator field.
Useful? React with 👍 / 👎.
| private func executionErrorBanner(tab: QueryTab) -> some View { | ||
| if let error = tab.execution.errorMessage, | ||
| tab.display.activeResultSet?.errorMessage == nil, | ||
| tab.display.resultSets.count <= 1 { |
There was a problem hiding this comment.
When a tab has a pinned result (or otherwise has more than one result set), this condition suppresses the new execution-level banner. The single-statement error path only sets tab.execution.errorMessage and leaves the existing display.resultSets in place, so after a failed run with pinned results the UI keeps showing the old results and no error banner, while the modal sheet was removed in this commit. The result-set count should not hide execution errors that are not represented by an active result set.
Useful? React with 👍 / 👎.
| let liveWidths = currentColumnWidths() | ||
| guard !liveWidths.isEmpty else { return saved } | ||
| return (saved ?? ColumnLayoutState()).mergingWidths(liveWidths) |
There was a problem hiding this comment.
This merges current NSTableView widths even on updates where the result schema changed. In applyStructuralUpdate, rebuildColumnMetadataCache(from:) has already swapped identitySchema to the new columns before this runs, while the table still contains slot-based columns from the previous result (dataColumn-0, etc.), so old widths are attributed to unrelated new column names by position. After resizing a column and running a different SELECT, the new result can inherit stale widths instead of its stored/default layout; only merge live widths when the schema is unchanged, or capture them before replacing the schema.
Useful? React with 👍 / 👎.
…-editor-filters # Conflicts: # CHANGELOG.md
There was a problem hiding this comment.
💡 Codex Review
https://github.com/TableProApp/TablePro/blob/5043f1e2f10e5b52295a5d11130cc76abfb3a363/Core/Coordinators/QueryExecutionCoordinator%2BHelpers.swift#L486-L488
When the user has toggled the results pane closed, single-statement failures are now invisible: this path only stores tab.execution.errorMessage, but the new banner is rendered in QuerySplitView's bottom content, which is hidden while tab.display.isResultsCollapsed is true. Since the modal alert was removed from this error path and success paths explicitly uncollapse results, set the tab and toolbar collapsed state to false when recording the error so the inline banner can actually be seen.
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
TablePro/TablePro/Core/Coordinators/QueryExecutionCoordinator+Helpers.swift
Lines 487 to 489 in 24a3e2e
Uncollapse the results pane for execution errors
When the user has collapsed the results pane, this error is now only rendered by resultsSection inside the collapsed QuerySplitView bottom item; the modal sheet that previously made the failure visible was removed in this change. Unlike successful executions, this error path does not set tab.display.isResultsCollapsed = false or update toolbarState.isResultsCollapsed, so a syntax/runtime failure can appear to do nothing until the user manually expands results.
ℹ️ 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".
| InlineErrorBanner( | ||
| message: error, | ||
| onFixWithAI: AppSettingsManager.shared.ai.enabled && tab.tabType == .query | ||
| ? { coordinator.fixErrorWithAI(query: tab.content.query, error: error) } |
There was a problem hiding this comment.
Fresh evidence in this revision is that the new banner callback passes the whole editor buffer, while runQuery() can execute only the selected text or the statement at the cursor. In a tab containing multiple statements, Fix with AI now receives unrelated SQL instead of the exact statement that failed, regressing the previous error handler's queryCopy = sql behavior; store the executed SQL with the error state and pass that here.
Useful? React with 👍 / 👎.
| if !latestRows.columns.isEmpty { | ||
| coordinator.isRebuildingColumns = true | ||
| let savedLayout = coordinator.savedColumnLayout(binding: columnLayout) | ||
| let savedLayout = coordinator.resolvedColumnLayout(binding: columnLayout, liveWidths: liveColumnWidths) |
There was a problem hiding this comment.
When updateNSView runs after switching to a different tab/table, liveColumnWidths still reflects the reused NSTableView's previous result, but this line merges those widths into the new grid's saved layout before reconciling columns. If the two results share common names such as id or name, the new tab/table ignores its persisted/default width and inherits the old tab's live width; merge live widths only when the prior schema/config is the same logical grid.
Useful? React with 👍 / 👎.
… drop the duplicate file Claude-Session: https://claude.ai/code/session_0198faM6VCrViRU4XwRoS1DC
datlechin
force-pushed
the
fix/ux-batch-datagrid-editor-filters
branch
from
1642d69 to
93e772a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93e772a688
ℹ️ 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 let error = tab.display.activeResultSet?.errorMessage ?? tab.execution.errorMessage { | ||
| InlineErrorBanner( | ||
| message: error, | ||
| onFixWithAI: AppSettingsManager.shared.ai.enabled && tab.tabType == .query |
There was a problem hiding this comment.
When a table-tab filter/raw-filter query fails, the tab rendering this banner has tab.tabType == .table, so this condition makes onFixWithAI nil even when AI is enabled. The old execution-error flow offered the AI fix action for these failures, and this change says filter errors should keep that opt-in action; with a malformed filter the banner now only allows copy/dismiss. Allow table/filter execution errors to supply the action too, using the generated failed SQL/filter context.
Useful? React with 👍 / 👎.
| let clamped = min(max(cursor, 0), nsText.length) | ||
| guard clamped > 0 else { return false } | ||
| guard let scalar = Unicode.Scalar(nsText.character(at: clamped - 1)) else { return true } | ||
| return !CharacterSet.whitespaces.contains(scalar) |
There was a problem hiding this comment.
Raw SQL filter fields are multiline (allowsMultiLine: true), but this check only uses CharacterSet.whitespaces, which does not contain \n. When the cursor is at the start of a new/blank line, the function returns true and still asks the SQL-token provider for completions with an empty token, so the popup can appear on empty space despite this fix; use .whitespacesAndNewlines (or include .newlines) before offering token completion.
Useful? React with 👍 / 👎.
1 participant
Fixes a batch of UX issues reported from real first-use feedback. Investigated with parallel codebase tracing across the data grid, SQL editor, filter bar, and the bottom toolbar.
Root cause: one bug behind three "stopped working mid-session" symptoms
The dead Columns / + Add buttons, the dead mouse in the SQL editor, and the general "unusable after a while" feeling were all the same bug. The SQL autocomplete popup (
SuggestionControllerin the vendored CodeEditSourceEditor) is a borderless non-activating panel at pop-up-menu level, added as a child window ordered above the main window. Its positioning only flipped the panel above the cursor when it would run off the screen bottom, never the editor's bottom. With the cursor on a lower line and the schema loaded, the panel drew straight down over the status bar and lower editor lines. Being non-activating, clicking it never changed first responder, so it never auto-closed and just ate every click until Esc.Fixes
Editor / autocomplete
FindPanelHostingViewthat leaked an Escape key monitor every time a tab was closed with the Find bar open.cancelHover()on the jump-to-definition failure path.Data grid
mouseDownnow bails to pure resize when the click starts in the resize zone).Filters
Not changed (by design)
Backspace deleting two characters is CodeEditSourceEditor's smart-edit behavior (delete a full indent unit in leading whitespace, or both halves of an auto-inserted bracket pair), the same as Xcode. Left as-is.
Tests
ColumnLayoutStateTests- geometry updates preserve the hidden-column set.FileColumnLayoutPersisterTests-cacheLayoutis in-memory only and is not written to disk.FilterValueTextFieldTests- whitespace suppresses completion; Escape grace keeps the filter bar open.The gesture/positioning fixes (header resize zone, panel geometry) are AppKit-window-bound and verified by hand rather than unit tested.
Notes
LocalPackages/CodeEditSourceEditor. They lint clean under that package's own config.https://claude.ai/code/session_0198faM6VCrViRU4XwRoS1DC