fix(editor): recheck query diagnostics whenever the document is replaced and show their messages - #2728
Merged
Merged
Conversation
…ced and show their messages Claude-Session: https://claude.ai/code/session_011THKc9TRHE8xjcxXidDHXP
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
This was referenced Sep 11, 2026
Merged
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.
Found while investigating #2717 (PR #2724).
The SQL editor underlines structural mistakes (an unclosed bracket, a stray quote) 500ms after typing stops. Three things were wrong with those underlines:
QueryDiagnostic.messagewas produced for every problem and never shown, andQueryDiagnosticsController.diagnostic(at:)had no caller.Root cause
Refresh. Diagnostics were scheduled only from
textView(_:didReplaceContentsIn:with:), which fires on edits. A tab switch or a loaded query replaces the whole document throughTextViewController.setText(the app's only route, viaTextBindingSync), and that method rebuilt the text storage, folds and highlighter without telling anyTextViewCoordinator. So nothing re-ran the check and nothing removed the old emphasis layers, which live on the text view's layer and survive a storage swap. The statement run controls had the same gap and were only kept current becauserepointFolds(to:)happened to refresh them as a side effect.Drawing.
EmphasisManager.createEmphasisLayerbuilt the shape layer only when the range already had line fragments. For a range on a line not yet laid out it returned a bareCAShapeLayer()that was never inserted into the view and had no stroke colour or line width.updateLayerBackgrounds()later gave that layer a path but never attached it, so the underline stayed invisible for good.Messages. Nothing carried the message from the diagnostic to the view.
What changed
TextViewCoordinatorgainstextViewDidReplaceDocument(controller:)with an empty default, andTextViewController.setTextcalls it on every coordinator after the new document is in place. This is the vendored CodeEditSourceEditor, not PluginKit, so no ABI concern.SQLEditorCoordinator.textViewDidReplaceDocumentdrops the old underlines at once, schedules a check of the new text (skipped past the same 100,000 character limit edits use), refreshes the run controls and current-statement highlight, and resets the Vim line cache, fold preview and inline suggestion. The run-control refresh moved here out ofrepointFolds(to:). A large document now also clears its underlines on edit instead of keeping stale ones, and teardown clears them too.EmphasisManager: the per-emphasis record is a class, so a layer that could not be drawn yet is created with its real style and attached on the firstupdateLayerBackgrounds()that finds its line fragments.Emphasisgains an optionaltoolTip.EmphasisManagerregisters nativeNSView.addToolTip(_:owner:userData:)rects for it through a newEmphasisToolTipsowner, moves them when layout moves the text, and removes them when the emphasis is removed, flashed away, or its text is gone. Query diagnostics pass their message, so resting the pointer on an underline shows it.QueryDiagnosticsRotorSearchadds anNSAccessibilityCustomRotornamed "Query Issues" to the editor's text view. VoiceOver walks the problems in document order, reads each message, and can filter by message text.install(on:)is idempotent.QueryDiagnosticsController.clear(in:)returns early when nothing is underlined, becauseremoveEmphases(for:)forces a layer layout and clear now runs on every document replacement.docs/features/sql-editor.mdxdescribes the tab-switch check, the tooltip and the rotor. One CHANGELOG line under Fixed.Verification
verify.sh build: succeeded (.analysis/fix-query-diagnostics-refresh/logs/build-TablePro-154839.log).verify.sh testover 13 suites (QueryDiagnosticsRefreshTests,QueryDiagnosticMessageTests,SQLEditorCoordinatorTests,SQLEditorCoordinatorCleanupTests,SQLEditorCoordinatorEscapeMenuTests,EditorLifecycleTeardownTests,RemoveInvisibleCharactersCommandTests,StatementRunControllerTests,StatementNavigationCommandTests,GutterHighlightTests,StatementRunPerformanceGuardTests,QueryCompletionAdapterLifecycleTests,StringCatalogIntegrityTests): 99 cases executed, 99 passed (.analysis/fix-query-diagnostics-refresh/logs/test-QueryDiagnosticsRefreshTests-154956.log).QueryDiagnosticsRefreshTests, 7 inQueryDiagnosticMessageTests), all passed (.analysis/fix-query-diagnostics-refresh/logs/test-QueryDiagnosticsRefreshTests-155454.log).setTextnotification removed, all 5 refresh cases that existed then failed (test-QueryDiagnosticsRefreshTests-145858.log,-145954.log). With the lazy attach and the early-returning clear reverted,underlineBelowTheViewportIsDrawnandclearWithoutUnderlinesDoesNotForceLayoutfailed (test-QueryDiagnosticsRefreshTests-155225.log).swift test --package-path LocalPackages/CodeEditTextView(the CI gate), rerun on the committed HEAD: XCTest 52 tests, 0 failures; Swift Testing 263 tests in 25 suites, all passed. That includes 6 newEmphasisManagerTestscases: the tooltip covers its text and nothing else, no tooltip without text, removal drops it, a deleted range drops it, it follows a layout change, and an underline on a line not yet laid out is drawn with its stroke once the line lays out.swiftlint lint --stricton every changed Swift file: no violations on added lines. The 12 it reports in the vendoredLocalPackagesfiles are all on lines that predate this branch.python3 scripts/localization.py verifyand the docs checks (check-writing-style.sh,check-docs-against-source.py) passed.TextViewController.setTextandTextBindingSyncpaths and ask the tooltip owner and the rotor delegate directly.Notes
Codex review was unavailable (usage limit), so an adversarial reviewer agent read the diff before the commit. On a final read for this PR, these were left as they are:
setTextStoragepath.TextView.setTextStoragestill does not notify coordinators. Nothing in the app replaces the storage that way; every document replacement goes throughTextViewController.setText.EmphasisLayeridentity. Changing it from a struct to a class makes==identity. The only comparison is the flash removal lookup, which already holds the same instance.https://claude.ai/code/session_011THKc9TRHE8xjcxXidDHXP