refactor: replace autocomplete NSTableView with native SwiftUI#375
Merged
refactor: replace autocomplete NSTableView with native SwiftUI#375
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughReplaced the AppKit autocomplete UI with SwiftUI components and moved matching/highlighting responsibilities into the provider: added matchedRanges to suggestion models, provider-driven filtering/ranking (including fuzzy indices), clause-aware suggestion caps, empty-prefix suppression in many contexts, and selection/theme state managed in the SuggestionViewModel with keyboard/first-responder handling in SuggestionController. Changes
Sequence DiagramsequenceDiagram
actor User
participant Editor as TextEditor
participant Controller as SuggestionController
participant Engine as CompletionEngine
participant Provider as SQLCompletionProvider
participant ViewModel as SuggestionViewModel
participant UI as SuggestionContentView
User->>Editor: Type character / move cursor
Editor->>Controller: requestCompletions(context)
Controller->>Engine: getCompletions(context)
Engine->>Provider: provider.filterAndRank(items, prefix, context)
Provider->>Provider: fuzzyMatchWithIndices() -> score + indices
Provider->>Provider: indicesToRanges() => matchedRanges
Provider-->>Engine: ranked items (with matchedRanges)
Engine-->>Controller: return completions
Controller->>ViewModel: update items (reset selectedIndex -> 0)
ViewModel-->>UI: publishes items & selectedIndex
UI->>UI: render rows (CodeSuggestionLabelView bolds matchedRanges)
User->>UI: Arrow keys / Escape / Return/Tab
UI->>ViewModel: moveUp()/moveDown()/applySelectedItem()
ViewModel-->>Editor: insert completion
Controller->>Controller: close suggestion window
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
LocalPackages/CodeEditSourceEditor/Sources/CodeEditSourceEditor/CodeSuggestion/View/SuggestionContentView.swift (2)
20-22: Simplify the pathComponents check.The current condition is verbose. Since
nil?.isEmptyevaluates tonil(and?? truemakes ittrue), you can simplify:♻️ Suggested simplification
if let item = model.selectedItem, item.documentation != nil || item.sourcePreview != nil - || (item.pathComponents != nil && !(item.pathComponents?.isEmpty ?? true)) { + || !(item.pathComponents?.isEmpty ?? true) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@LocalPackages/CodeEditSourceEditor/Sources/CodeEditSourceEditor/CodeSuggestion/View/SuggestionContentView.swift` around lines 20 - 22, The conditional that checks item.pathComponents is overly verbose; replace the current `(item.pathComponents != nil && !(item.pathComponents?.isEmpty ?? true))` with a simpler optional check such as `item.pathComponents?.isEmpty == false` (or `!(item.pathComponents?.isEmpty ?? true)`) inside the existing `if let item = model.selectedItem` condition to preserve the same semantics; update the condition around model.selectedItem / item.pathComponents in SuggestionContentView.swift accordingly.
91-94: Avoidstring.counton potentially large strings.
$1.label.countand$1.detail?.countare O(n) operations on Swift strings. While the width result is capped at 64, the full string length is still computed. For suggestion labels this is typically fine, but favorites can contain full SQL queries.Consider using
(label as NSString).lengthfor O(1) access:♻️ Suggested fix
let maxLabelLength = min( - (model.items.reduce(0) { max($0, $1.label.count + ($1.detail?.count ?? 0)) }) + 2, + (model.items.reduce(0) { + let labelLen = ($1.label as NSString).length + let detailLen = ($1.detail as NSString?)?.length ?? 0 + return max($0, labelLen + detailLen) + }) + 2, 64 )As per coding guidelines: "Never use string.count on large strings — O(n) in Swift. Use (string as NSString).length for O(1)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@LocalPackages/CodeEditSourceEditor/Sources/CodeEditSourceEditor/CodeSuggestion/View/SuggestionContentView.swift` around lines 91 - 94, The current reduce calculating maxLabelLength uses Swift String.count (in the closure referencing $1.label.count and $1.detail?.count) which is O(n) and can be expensive for large suggestion strings; update the closure inside the computation of maxLabelLength in SuggestionContentView (the reduce over model.items) to use (label as NSString).length and (detail as NSString).length (safely handling optional detail) instead of .count so length checks are O(1), preserving the +2 and min(..., 64) logic.TableProTests/Views/Editor/SQLCompletionAdapterFuzzyTests.swift (1)
15-27: Consider caching the provider instance.A new
SQLCompletionProvideris created for eachfuzzyMatchcall. SincefuzzyMatchScoreis pure and doesn't touch schema state (as noted in the comment), you could make the provider a shared instance within the test struct:♻️ Optional refactor
struct SQLCompletionAdapterFuzzyTests { + private let provider = SQLCompletionProvider(schemaProvider: SQLSchemaProvider()) + /// Helper: wraps SQLCompletionProvider.fuzzyMatchScore as a bool match /// to preserve existing test semantics after the fuzzy logic was unified. private func fuzzyMatch(pattern: String, target: String) -> Bool { - let provider = makeDummyProvider() // Empty pattern is a vacuous match if pattern.isEmpty { return true } return provider.fuzzyMatchScore(pattern: pattern, target: target) != nil } - - private func makeDummyProvider() -> SQLCompletionProvider { - // Provider only needs schemaProvider for candidate generation, - // fuzzyMatchScore is pure and doesn't touch the schema. - let schema = SQLSchemaProvider() - return SQLCompletionProvider(schemaProvider: schema) - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TableProTests/Views/Editor/SQLCompletionAdapterFuzzyTests.swift` around lines 15 - 27, Tests currently create a new SQLCompletionProvider on every fuzzyMatch call; make a single shared provider instance to avoid repeated construction by adding a stored property (e.g., private lazy var sharedProvider: SQLCompletionProvider or private let sharedProvider = SQLCompletionProvider(...)) inside the test struct and update fuzzyMatch(pattern:target:) to use sharedProvider instead of calling makeDummyProvider(); you can then either remove makeDummyProvider() or have it return sharedProvider so references like SQLCompletionProvider(schemaProvider:) and fuzzyMatchScore(pattern:target:) are easy to locate.LocalPackages/CodeEditSourceEditor/Sources/CodeEditSourceEditor/CodeSuggestion/View/CodeSuggestionLabelView.swift (2)
11-18: Make the new view’s access level explicit.Please spell out
internal/publiconCodeSuggestionLabelViewso the package surface stays intentional.As per coding guidelines, "Always specify access control explicitly (private, internal, public) on extensions and types."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@LocalPackages/CodeEditSourceEditor/Sources/CodeEditSourceEditor/CodeSuggestion/View/CodeSuggestionLabelView.swift` around lines 11 - 18, The struct CodeSuggestionLabelView currently lacks an explicit access level; update its declaration to include the intended access modifier (e.g., add "internal" or "public" before "struct CodeSuggestionLabelView") so the package surface is explicit, and ensure any members that need broader visibility (like properties suggestion, labelColor, secondaryLabelColor, font, isSelected and HORIZONTAL_PADDING) are adjusted if their access must differ from the struct's default.
47-65: Avoid repeatedStringwalks in the highlight loop.This path recalculates
label.countandlabel.index(_:offsetBy:)for every matched range. On longer labels that turns row rendering into repeated O(n) scans; precompute the length once and avoid per-segmentoffsetBywalks here.As per coding guidelines, "Never use string.count on large strings — O(n) in Swift. Use (string as NSString).length for O(1)." and "Never use string.index(string.startIndex, offsetBy:) in loops on bridged NSStrings — O(n) per call."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@LocalPackages/CodeEditSourceEditor/Sources/CodeEditSourceEditor/CodeSuggestion/View/CodeSuggestionLabelView.swift` around lines 47 - 65, Precompute an NSString bridge and its length and use NSString substring(with:) instead of repeatedly calling label.count and label.index(_:offsetBy:): create let nsLabel = label as NSString and let labelLength = nsLabel.length, use labelLength for clampedUpper comparisons, and inside the loop produce segment strings with nsLabel.substring(with: NSRange(location: range.lowerBound, length: clampedUpper - range.lowerBound)) (and similarly for the non-highlighted segments using ranges computed from currentIndex) and wrap those substrings as String for the Text() initializers; update uses of currentIndex, clampedUpper and ranges to operate on these integer offsets so you avoid per-iteration O(n) String.index walks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@LocalPackages/CodeEditSourceEditor/Sources/CodeEditSourceEditor/CodeSuggestion/Model/SuggestionViewModel.swift`:
- Around line 110-112: When replacing self.items, resetting self.selectedIndex =
0 and clearing syntaxHighlightedCache, also notify the delegate by calling
completionWindowDidSelect(item:) with the new first item (guarding for empty
items) so the preview/metadata update immediately; apply the same change to the
other spot that sets items/selectedIndex/syntaxHighlightedCache (the second
replacement block) to ensure both refresh paths trigger the delegate update.
- Around line 164-167: In willClose(), cancel and nil out the outstanding Task
that fetches completions (itemsRequestTask) before clearing state so it cannot
repopulate items or call showWindowOnParent after the popup is closed;
specifically, call itemsRequestTask?.cancel(), set itemsRequestTask = nil, then
clear items/selectedIndex/activeTextView, and ensure any completion handlers
(where showWindowOnParent is invoked) early‑return if the Task was cancelled.
In
`@LocalPackages/CodeEditSourceEditor/Sources/CodeEditSourceEditor/CodeSuggestion/Window/SuggestionController.swift`:
- Around line 136-145: The observer added with
NotificationCenter.default.addObserver for NSWindow.didUpdateNotification
(assigned to firstResponderObserver) only checks whether
model.activeTextView?.view.window is nil and thus keeps the suggestion UI open
when focus moves away within the same window; update the callback to also verify
that the active editor is still the window’s first responder (e.g. compare
parentWindow.firstResponder or check
model.activeTextView?.view.isFirstResponder) and call close() when it is not;
apply the same change to the other observer block covering the 199-228 region so
both observers close the suggestion UI on focus loss, not just window
detachment.
In `@TablePro/Core/Autocomplete/SQLCompletionProvider.swift`:
- Around line 496-499: In filterAndRank(_:prefix:context:) clear any existing
highlight state on the items before recomputing matches: iterate the filtered
[SQLCompletionItem] and reset each item's matchedRanges (or equivalent property)
to an empty array/string so stale highlights are removed when prefix is empty or
shortened, then call populateMatchRanges(&filtered, prefix: prefix) and continue
with rankResults(filtered, prefix: prefix, context: context); this ensures
SQLCompletionItem.matchedRanges is always reset prior to recomputation.
- Around line 577-620: The fuzzyMatchWithIndices function currently collects
matchedIndices using NSString.character(at:) (UTF-16 offsets) which mismatches
the other branches that use Swift String character boundaries; update
fuzzyMatchWithIndices to return indices aligned to String.CharacterView
positions by converting each UTF-16 index in matchedIndices to the corresponding
String character index (or refactor the matching loop to operate over Swift
String characters instead of NSString), ensuring matchedIndices are in the same
character-index space used elsewhere and keeping the returned (score,
matchedIndices) shape.
---
Nitpick comments:
In
`@LocalPackages/CodeEditSourceEditor/Sources/CodeEditSourceEditor/CodeSuggestion/View/CodeSuggestionLabelView.swift`:
- Around line 11-18: The struct CodeSuggestionLabelView currently lacks an
explicit access level; update its declaration to include the intended access
modifier (e.g., add "internal" or "public" before "struct
CodeSuggestionLabelView") so the package surface is explicit, and ensure any
members that need broader visibility (like properties suggestion, labelColor,
secondaryLabelColor, font, isSelected and HORIZONTAL_PADDING) are adjusted if
their access must differ from the struct's default.
- Around line 47-65: Precompute an NSString bridge and its length and use
NSString substring(with:) instead of repeatedly calling label.count and
label.index(_:offsetBy:): create let nsLabel = label as NSString and let
labelLength = nsLabel.length, use labelLength for clampedUpper comparisons, and
inside the loop produce segment strings with nsLabel.substring(with:
NSRange(location: range.lowerBound, length: clampedUpper - range.lowerBound))
(and similarly for the non-highlighted segments using ranges computed from
currentIndex) and wrap those substrings as String for the Text() initializers;
update uses of currentIndex, clampedUpper and ranges to operate on these integer
offsets so you avoid per-iteration O(n) String.index walks.
In
`@LocalPackages/CodeEditSourceEditor/Sources/CodeEditSourceEditor/CodeSuggestion/View/SuggestionContentView.swift`:
- Around line 20-22: The conditional that checks item.pathComponents is overly
verbose; replace the current `(item.pathComponents != nil &&
!(item.pathComponents?.isEmpty ?? true))` with a simpler optional check such as
`item.pathComponents?.isEmpty == false` (or `!(item.pathComponents?.isEmpty ??
true)`) inside the existing `if let item = model.selectedItem` condition to
preserve the same semantics; update the condition around model.selectedItem /
item.pathComponents in SuggestionContentView.swift accordingly.
- Around line 91-94: The current reduce calculating maxLabelLength uses Swift
String.count (in the closure referencing $1.label.count and $1.detail?.count)
which is O(n) and can be expensive for large suggestion strings; update the
closure inside the computation of maxLabelLength in SuggestionContentView (the
reduce over model.items) to use (label as NSString).length and (detail as
NSString).length (safely handling optional detail) instead of .count so length
checks are O(1), preserving the +2 and min(..., 64) logic.
In `@TableProTests/Views/Editor/SQLCompletionAdapterFuzzyTests.swift`:
- Around line 15-27: Tests currently create a new SQLCompletionProvider on every
fuzzyMatch call; make a single shared provider instance to avoid repeated
construction by adding a stored property (e.g., private lazy var sharedProvider:
SQLCompletionProvider or private let sharedProvider =
SQLCompletionProvider(...)) inside the test struct and update
fuzzyMatch(pattern:target:) to use sharedProvider instead of calling
makeDummyProvider(); you can then either remove makeDummyProvider() or have it
return sharedProvider so references like SQLCompletionProvider(schemaProvider:)
and fuzzyMatchScore(pattern:target:) are easy to locate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f1c85a41-72b2-44aa-acbd-bc2621e3e93b
📒 Files selected for processing (18)
CHANGELOG.mdLocalPackages/CodeEditSourceEditor/Sources/CodeEditSourceEditor/CodeSuggestion/Model/CodeSuggestionEntry.swiftLocalPackages/CodeEditSourceEditor/Sources/CodeEditSourceEditor/CodeSuggestion/Model/SuggestionViewModel.swiftLocalPackages/CodeEditSourceEditor/Sources/CodeEditSourceEditor/CodeSuggestion/TableView/CodeSuggestionLabelView.swiftLocalPackages/CodeEditSourceEditor/Sources/CodeEditSourceEditor/CodeSuggestion/TableView/CodeSuggestionPreviewView.swiftLocalPackages/CodeEditSourceEditor/Sources/CodeEditSourceEditor/CodeSuggestion/TableView/CodeSuggestionRowView.swiftLocalPackages/CodeEditSourceEditor/Sources/CodeEditSourceEditor/CodeSuggestion/TableView/NoSlotScroller.swiftLocalPackages/CodeEditSourceEditor/Sources/CodeEditSourceEditor/CodeSuggestion/TableView/SuggestionViewController.swiftLocalPackages/CodeEditSourceEditor/Sources/CodeEditSourceEditor/CodeSuggestion/View/CodeSuggestionLabelView.swiftLocalPackages/CodeEditSourceEditor/Sources/CodeEditSourceEditor/CodeSuggestion/View/SuggestionContentView.swiftLocalPackages/CodeEditSourceEditor/Sources/CodeEditSourceEditor/CodeSuggestion/View/SuggestionPreviewView.swiftLocalPackages/CodeEditSourceEditor/Sources/CodeEditSourceEditor/CodeSuggestion/Window/SuggestionController+Window.swiftLocalPackages/CodeEditSourceEditor/Sources/CodeEditSourceEditor/CodeSuggestion/Window/SuggestionController.swiftTablePro/Core/Autocomplete/CompletionEngine.swiftTablePro/Core/Autocomplete/SQLCompletionItem.swiftTablePro/Core/Autocomplete/SQLCompletionProvider.swiftTablePro/Views/Editor/SQLCompletionAdapter.swiftTableProTests/Views/Editor/SQLCompletionAdapterFuzzyTests.swift
💤 Files with no reviewable changes (5)
- LocalPackages/CodeEditSourceEditor/Sources/CodeEditSourceEditor/CodeSuggestion/TableView/NoSlotScroller.swift
- LocalPackages/CodeEditSourceEditor/Sources/CodeEditSourceEditor/CodeSuggestion/TableView/CodeSuggestionLabelView.swift
- LocalPackages/CodeEditSourceEditor/Sources/CodeEditSourceEditor/CodeSuggestion/TableView/CodeSuggestionRowView.swift
- LocalPackages/CodeEditSourceEditor/Sources/CodeEditSourceEditor/CodeSuggestion/TableView/SuggestionViewController.swift
- LocalPackages/CodeEditSourceEditor/Sources/CodeEditSourceEditor/CodeSuggestion/TableView/CodeSuggestionPreviewView.swift
...EditSourceEditor/Sources/CodeEditSourceEditor/CodeSuggestion/Model/SuggestionViewModel.swift
Show resolved
Hide resolved
...EditSourceEditor/Sources/CodeEditSourceEditor/CodeSuggestion/Model/SuggestionViewModel.swift
Show resolved
Hide resolved
...itSourceEditor/Sources/CodeEditSourceEditor/CodeSuggestion/Window/SuggestionController.swift
Show resolved
Hide resolved
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
1 participant
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.
Summary
List(selection:)hosted in the existing NSPanelselectionHighlightStyle = .none+ unreliable customdrawBackground())Files deleted (5): SuggestionViewController, CodeSuggestionRowView, CodeSuggestionPreviewView, NoSlotScroller (replaced by SwiftUI equivalents)
Files created (3): SuggestionContentView (SwiftUI List), SuggestionPreviewView (SwiftUI), CodeSuggestionLabelView (moved + enhanced with isSelected)
Net: -93 lines (618 added, 711 removed)
Test plan
FROMshows table list (empty prefix allowed in FROM context)Summary by CodeRabbit
New Features
Improvements