fix: autocomplete after reconnect, JSON results view refresh and selection, double-click word selection - #2047
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Signed-off-by: Ngô Quốc Đạt <datlechin@gmail.com>
This was referenced Aug 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.
Fixes four reported bugs. Each is a separate commit; the root causes are unrelated.
1. Autocomplete stops suggesting tables and columns
ConnectionHealthMonitor's background auto-reconnect calledSchemaService.invalidate(connectionId:), reconnected, and returned success without publishingdatabaseDidConnect. The first-connect path and the manual reconnect path both publish it.Two consequences.
invalidateis reserved by the "A refresh never clears the cache it is refreshing" invariant for genuine teardown, and a silent background reconnect is not teardown. And with no event published, neither listener runs, sosyncAutocompleteProvider(the only writer of the autocomplete schema) never runs again. The window still reads as connected while table and column completion is dead for the rest of the session, which is why it correlates with leaving a connection in the background and coming back to it.invalidatebecomesprepareForReloadon both reconnect paths, matching every other caller and the adjacentDatabaseTreeMetadataService.handleReconnect.databaseDidConnect.performHealthMonitorReconnect(connectionId:). It was an anonymous closure literal with no test reaching it, which is how this shipped.syncAutocompleteProvider's three-way guard logged nothing on any of its exits. It now logs each, and reads the browse scope through the already-injectedmetadataDriverProviderso tests can reach it.SchemaRefreshServiceTestspasseddatabaseManager: nil, so that guard had never been exercised.2. The JSON results view does not refresh on a new query
TableRowsis a value struct replaced wholesale per query, andResultsJsonViewgated its rebuild on.onChange(of: tableRows.count). A new result with the same row count changes neither the count nor the selection, so nothing fires. The data grid never showed this becausesetActiveTableRowsimperatively callsapplyFullReplace(); the JSON view had no equivalent.TabSessiongainsdataRevision, bumped byTabSessionRegistryon every row mutation, so incremental cell edits count as changes too. The view swaps threeonChangemodifiers and a hand-rolledrenderTokenfor a single.task(id:), which is the documented API for this ("If theidvalue changes, SwiftUI cancels and restarts the task") and removes the manual out-of-order guard.3. The JSON view shows
[]unless a row is selectedTwo independent defects.
GridSelectionStateis one shared instance per window and was never reset when a query ran, so indices from the previous result survived.computeJsonthen dropped every one of them as out of range and rendered[]. The reset now happens at the mutation chokepoint (setActiveTableRows/switchActiveResultSet) rather than in the view, because the row details inspector reads the same shared state and had the same latent bug. Incremental edits go throughmutateActiveTableRowsand keep their selection, soRowEditingCoordinator's deliberate post-delete selection is untouched.Separately,
computeJsonsubscriptedTableRows.rowswith selection indices. Those are display positions ("Selection indices are display positions", the #1837 bug class), so with a per-column value filter active the JSON view showed the wrong rows even with nothing stale. It now resolves throughDisplayRowMapping, and the row count label reports what actually resolved instead of the raw selection size.The grid also deselects on a wholesale replace:
reloadData()leavesselectedRowIndexesalone when the new result happens to have as many rows as the old one.4. Double-click does not select a word
Three defects in the vendored
CodeEditTextView, all reachable from normal use:selectWord(_:)usedcompactMapwithguard textSelection.range.isEmpty else { return nil }, so any non-empty selection was dropped. Double-clicking while text was selected cleared the selection entirely, and sincecompactMapover an empty array stays empty, double-click then stayed dead until a plain single click.DragSelectionGestureholds back the first click of the pair when it lands inside an existing selection, which is how the second click arrived with stale state.findWordBoundaryreturned an empty range for the last word in the document, becausefindNextOccurrenceOfCharacterreturns nil once it runs off the end. In a SQL editor that is usually the table name inFROM users. Both document edges are now boundaries, which is whatTextFormationalready does for the same call.handleSingleClickbailed out before setting any selection when!isEditable, so a read-only editor never placed a caret. The JSON results Text view is exactly that. Selection placement is now gated onisSelectable, andTextSelectionManagersuppresses the blinking insertion point when the view is not editable, mirroringNSTextViewwithisEditable = falseandisSelectable = true.Double and triple click now resolve the word or line from the clicked offset instead of from whatever the selection happened to be, which is how AppKit derives granularity from click count. That also makes the drag-gesture interaction harmless.
setSelectedRange(s)clamps out-of-bounds ranges instead of discarding them, closing a third route into the "no selection at all" state that a shorter document could trigger on a tab switch.The result grid is deliberately unchanged.
DataGridCellViewdraws withCTLineDrawand has no text substrate; double-click already opens a real selectableNSTextViewoverlay (#1336). No mature client (TablePlus, DataGrip, Postico, Sequel Ace, Beekeeper) makes read-only grid cells raw-text-selectable, and doing so would conflict with cell-range selection and the 500+ column budget.Tests
HealthMonitorReconnectTests(new): the schema survives a reconnect, a successful reconnect publishes the event, a missing session aborts.SchemaRefreshServiceTests: the autocomplete provider actually receives the loaded tables, and a missing browse scope leaves it alone.ResultsJsonViewTests(new, this view had zero coverage): no selection renders everything, a selection resolves through display order, out-of-range indices are skipped, a selection resolving to nothing reports zero.MainContentCoordinatorSelectionResetTests(new): a new result clears live and persisted selection, a background tab does not disturb the foreground one, an incremental edit keeps its selection.TabSessionRegistryTests: every row mutation bumpsdataRevision.WordSelectionTests(new) andTextSelectionManagerTestsin the editor package. The last-word defect was found by these tests, not by reading.LocalPackages/CodeEditTextViewtests were not run by any workflow, so its regression tests would not have gated anything.macos-tests.ymlnow runs them: 172 tests green.Notes
Two adjacent problems found and deliberately left out of scope:
SourceEditor.updateControllerWithStatecomparescursorPositions != state.cursorPositionsagainst itself, so it is always false and the framework's downward cursor push is dead code. The visible effect is that TablePro's saved cursor restore never reaches the text view. Fixing it activates a path that has never run in production and could race live typing, so it deserves its own change and test pass.