-
-
Notifications
You must be signed in to change notification settings - Fork 297
refactor: remove dead code and consolidate SQL escaping #1732
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,7 @@ | |
| import AppKit | ||
| import CodeEditSourceEditor | ||
| import CodeEditTextView | ||
| import os | ||
| import SwiftUI | ||
|
|
||
| /// Adapts the existing CompletionEngine to CodeEditSourceEditor's suggestion system | ||
|
|
@@ -29,6 +30,13 @@ final class SQLCompletionAdapter: CodeSuggestionDelegate { | |
| private var favoriteKeywords: [String: (name: String, query: String)] = [:] | ||
| private var session: CompletionSession? | ||
| private let debounceNanoseconds: UInt64 = 50_000_000 | ||
| private let refilterDebounceNanoseconds: UInt64 = 30_000_000 | ||
|
|
||
| private var cursorRefilterTask: Task<Void, Never>? | ||
| private var lastRefilterPrefix: String? | ||
| private var lastRefilterItems: [SQLCompletionItem]? | ||
|
|
||
| private static let logger = Logger(subsystem: "com.TablePro", category: "SQLCompletionAdapter") | ||
|
|
||
| // MARK: - Initialization | ||
|
|
||
|
|
@@ -133,6 +141,10 @@ final class SQLCompletionAdapter: CodeSuggestionDelegate { | |
| } | ||
|
|
||
| // Adjust replacement range from window-relative back to document coordinates | ||
| cursorRefilterTask?.cancel() | ||
| cursorRefilterTask = nil | ||
| lastRefilterPrefix = nil | ||
| lastRefilterItems = nil | ||
| session = CompletionSession( | ||
| phase: .final, | ||
| context: CompletionContext( | ||
|
|
@@ -201,13 +213,83 @@ final class SQLCompletionAdapter: CodeSuggestionDelegate { | |
|
|
||
| guard !currentPrefix.isEmpty else { return nil } | ||
|
|
||
| let ranked = provider.filterAndRank(context.items, prefix: currentPrefix, context: context.sqlContext) | ||
| let synchronousItems = synchronousRefilter( | ||
| provider: provider, | ||
| fullItems: context.items, | ||
| sqlContext: context.sqlContext, | ||
| prefix: currentPrefix | ||
| ) | ||
|
|
||
| scheduleRefilterTask( | ||
| provider: provider, | ||
| fullItems: context.items, | ||
| sqlContext: context.sqlContext, | ||
| prefix: currentPrefix | ||
| ) | ||
|
|
||
| return synchronousItems?.map { SQLSuggestionEntry(item: $0) } | ||
| } | ||
|
|
||
| private func synchronousRefilter( | ||
| provider: SQLCompletionProvider, | ||
| fullItems: [SQLCompletionItem], | ||
| sqlContext: SQLContext, | ||
| prefix: String | ||
| ) -> [SQLCompletionItem]? { | ||
| if prefix == lastRefilterPrefix, let cached = lastRefilterItems { | ||
| return cached | ||
| } | ||
|
|
||
| if let lastPrefix = lastRefilterPrefix, | ||
| prefix.hasPrefix(lastPrefix), | ||
| let lastItems = lastRefilterItems { | ||
| let narrowed = provider.filterByPrefix(lastItems, prefix: prefix) | ||
| return narrowed.isEmpty ? nil : narrowed | ||
| } | ||
|
|
||
| let seeded = provider.filterByPrefix(fullItems, prefix: prefix) | ||
| return seeded.isEmpty ? nil : seeded | ||
| } | ||
|
|
||
| private func scheduleRefilterTask( | ||
| provider: SQLCompletionProvider, | ||
| fullItems: [SQLCompletionItem], | ||
| sqlContext: SQLContext, | ||
| prefix: String | ||
| ) { | ||
| cursorRefilterTask?.cancel() | ||
|
|
||
| cursorRefilterTask = Task { [weak self] in | ||
| guard let self else { return } | ||
|
|
||
| return ranked.isEmpty ? nil : ranked.map { SQLSuggestionEntry(item: $0) } | ||
| do { | ||
| try await Task.sleep(nanoseconds: self.refilterDebounceNanoseconds) | ||
| } catch { | ||
| return | ||
| } | ||
| guard !Task.isCancelled else { return } | ||
|
|
||
| let ranked = await Task.detached(priority: .userInitiated) { | ||
| provider.filterAndRank(fullItems, prefix: prefix, context: sqlContext) | ||
| }.value | ||
|
|
||
| guard !Task.isCancelled else { return } | ||
|
|
||
| await MainActor.run { | ||
| guard !Task.isCancelled else { return } | ||
| self.lastRefilterPrefix = prefix | ||
| self.lastRefilterItems = ranked | ||
|
Comment on lines
+280
to
+281
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In Useful? React with 👍 / 👎. |
||
| Self.logger.debug("refilter cached prefix='\(prefix)' count=\(ranked.count)") | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func completionWindowDidClose() { | ||
| session = nil | ||
| cursorRefilterTask?.cancel() | ||
| cursorRefilterTask = nil | ||
| lastRefilterPrefix = nil | ||
| lastRefilterItems = nil | ||
| } | ||
|
|
||
| func completionWindowApplyCompletion( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,100 @@ | ||
| // | ||
| // SQLCompletionProviderConcurrencyTests.swift | ||
| // TableProTests | ||
| // | ||
| // Guards the invariant that filterByPrefix and filterAndRank are pure and | ||
| // safe to call off the main actor. SQLCompletionAdapter runs filterAndRank | ||
| // on a detached task while typing, so concurrent invocations from the main | ||
| // actor's synchronous fast path must not diverge. | ||
| // | ||
|
|
||
| import TableProPluginKit | ||
| @testable import TablePro | ||
| import Testing | ||
|
|
||
| @Suite("SQL Completion Provider Concurrency") | ||
| struct SQLCompletionProviderConcurrencyTests { | ||
| private func makeProvider() -> SQLCompletionProvider { | ||
| SQLCompletionProvider(schemaProvider: SQLSchemaProvider()) | ||
| } | ||
|
|
||
| private func makeItems() -> [SQLCompletionItem] { | ||
| ["select", "set", "session", "schema", "savepoint", "score", "scalar"] | ||
| .map { SQLCompletionItem.keyword($0) } | ||
| } | ||
|
|
||
| private func makeContext(prefix: String) -> SQLContext { | ||
| SQLContext( | ||
| clauseType: .unknown, | ||
| prefix: prefix, | ||
| prefixRange: 0..<prefix.count, | ||
| dotPrefix: nil, | ||
| tableReferences: [], | ||
| isInsideString: false, | ||
| isInsideComment: false | ||
| ) | ||
| } | ||
|
|
||
| @Test("filterAndRank returns identical results across repeated invocations") | ||
| func filterAndRankIsDeterministic() { | ||
| let provider = makeProvider() | ||
| let items = makeItems() | ||
| let context = makeContext(prefix: "s") | ||
|
|
||
| let first = provider.filterAndRank(items, prefix: "s", context: context) | ||
| let second = provider.filterAndRank(items, prefix: "s", context: context) | ||
|
|
||
| #expect(first == second) | ||
| #expect(!first.isEmpty) | ||
| } | ||
|
|
||
| @Test("filterByPrefix result is a superset of filterAndRank matches") | ||
| func filterByPrefixSupersetOfRanked() { | ||
| let provider = makeProvider() | ||
| let items = makeItems() | ||
|
|
||
| let filtered = provider.filterByPrefix(items, prefix: "se") | ||
| let ranked = provider.filterAndRank(items, prefix: "se", context: makeContext(prefix: "se")) | ||
|
|
||
| let filteredLabels = Set(filtered.map { $0.label }) | ||
| let rankedLabels = Set(ranked.map { $0.label }) | ||
|
|
||
| #expect(rankedLabels.isSubset(of: filteredLabels)) | ||
| } | ||
|
|
||
| @Test("filterAndRank is safe under concurrent invocations from a detached task") | ||
| func filterAndRankConcurrent() async { | ||
| let provider = makeProvider() | ||
| let items = makeItems() | ||
| let context = makeContext(prefix: "sc") | ||
|
|
||
| let baseline = provider.filterAndRank(items, prefix: "sc", context: context) | ||
|
|
||
| await withTaskGroup(of: [SQLCompletionItem].self) { group in | ||
| for _ in 0..<8 { | ||
| group.addTask { | ||
| provider.filterAndRank(items, prefix: "sc", context: context) | ||
| } | ||
| } | ||
| for await result in group { | ||
| #expect(result == baseline) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @Test("filterByPrefix narrows correctly when prefix extends") | ||
| func filterByPrefixNarrowsOnExtension() { | ||
| let provider = makeProvider() | ||
| let items = makeItems() | ||
|
|
||
| let short = provider.filterByPrefix(items, prefix: "s") | ||
| let extended = provider.filterByPrefix(short, prefix: "se") | ||
|
|
||
| let direct = provider.filterByPrefix(items, prefix: "se") | ||
| let directLabels = Set(direct.map { $0.label }) | ||
| let extendedLabels = Set(extended.map { $0.label }) | ||
|
|
||
| #expect(extendedLabels == directLabels) | ||
| #expect(extended.count <= short.count) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When typing just slowly enough for the 30 ms debounce to elapse, canceling
cursorRefilterTaskon the next cursor move does not cancel the unstructuredTask.detachedspawned here. StalefilterAndRankcalls can continue over the full suggestion list while newer prefixes start their own work, so CPU-heavy refilters pile up in the large-list fast-typing case this change is meant to fix; keep the ranking in the cancellable task or retain and cancel the detached handle.Useful? React with 👍 / 👎.