fix: refactor autocomplete system for reliability#384
Merged
Conversation
- Strip .capsLock and .function from modifier flags so Escape/Ctrl+Space work on all keyboards - Remove weak var on SourceEditor struct delegates to prevent nil reference issues - Cancel in-flight completion tasks instead of blocking new triggers - Replace NSWindow with NSPanel subclass to fix canBecomeKeyWindow warnings - Add isManualTrigger flag so Ctrl+Space shows completions even with empty prefix - Wire up updateSchemaProvider on connection change (was dead code) - Add schema load retry with 30s cooldown for failed initial loads - Add diagnostic logging at key completion pipeline failure points
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds manual vs automatic completion trigger signaling, logging across suggestion flow, switches suggestion window from NSWindow to NSPanel, changes two delegates to strong references, and implements conditional schema-retry with cooldown for SQL completions. Changes
Sequence DiagramsequenceDiagram
participant User
participant TextView as TextViewController
participant SuggestCtrler as SuggestionController
participant SuggestModel as SuggestionViewModel
participant Adapter as SQLCompletionAdapter
participant Engine as CompletionEngine
participant SchemaProvider as SQLSchemaProvider
rect rgba(0, 150, 200, 0.5)
note over User,SchemaProvider: Manual Trigger Flow
User->>TextView: Press completion shortcut
TextView->>SuggestCtrler: showCompletions(isManualTrigger: true)
SuggestCtrler->>SuggestModel: showCompletions(..., isManualTrigger: true)
SuggestModel->>Adapter: completionSuggestionsRequested(..., isManualTrigger: true)
Adapter->>Engine: retrySchemaIfNeeded()
Engine->>SchemaProvider: retryLoadSchemaIfNeeded()
alt Schema retry successful or not needed
SchemaProvider-->>Engine: ✓
Engine-->>Adapter: ✓
end
Adapter->>Engine: getCompletions()
Engine-->>Adapter: Completion items
Adapter-->>SuggestModel: Return items (no empty-prefix suppression)
SuggestModel->>SuggestModel: Show completion window
end
rect rgba(100, 150, 50, 0.5)
note over User,SchemaProvider: Automatic Trigger Flow
User->>TextView: Type completion character
TextView->>SuggestCtrler: showCompletions(isManualTrigger: false)
SuggestCtrler->>SuggestModel: showCompletions(..., isManualTrigger: false)
SuggestModel->>Adapter: completionSuggestionsRequested(..., isManualTrigger: false)
Adapter->>Engine: retrySchemaIfNeeded()
Engine->>SchemaProvider: retryLoadSchemaIfNeeded()
SchemaProvider-->>Engine: ✓
Engine-->>Adapter: ✓
Adapter->>Engine: getCompletions()
Engine-->>Adapter: Completion items
Adapter->>Adapter: Filter: suppress empty-prefix items
Adapter-->>SuggestModel: Return filtered items
SuggestModel->>SuggestModel: Show completion window
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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: 1
🧹 Nitpick comments (2)
LocalPackages/CodeEditSourceEditor/Sources/CodeEditSourceEditor/CodeSuggestion/Model/SuggestionViewModel.swift (1)
13-13: Consider using a package-appropriate logger subsystem.This file is in the
CodeEditSourceEditorpackage, but the logger uses the"com.TablePro"subsystem. If this package is intended to be reusable or shared, a package-specific subsystem (e.g.,"com.CodeEditSourceEditor") would be more appropriate for filtering and identification.However, if this package is exclusively used within TablePro and you want unified log filtering, the current approach is acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@LocalPackages/CodeEditSourceEditor/Sources/CodeEditSourceEditor/CodeSuggestion/Model/SuggestionViewModel.swift` at line 13, The logger subsystem string in the static logger declaration uses "com.TablePro" which is package-mismatched; update the Logger initialization in SuggestionViewModel (the private static let logger) to use a package-appropriate subsystem such as "com.CodeEditSourceEditor" (or another package-specific identifier) so logs are correctly categorized for this package while keeping the category "SuggestionVM" unchanged.LocalPackages/CodeEditSourceEditor/Sources/CodeEditSourceEditor/CodeSuggestion/Window/SuggestionController+Window.swift (1)
11-14: Add explicit access control toSuggestionPanel.Per coding guidelines, access control should be explicitly specified on types. Since this panel is only used internally by
SuggestionController, it should be markedprivateorinternal.♻️ Suggested fix
-final class SuggestionPanel: NSPanel { +private final class SuggestionPanel: NSPanel { override var canBecomeKey: Bool { false } override var canBecomeMain: Bool { false } }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/Window/SuggestionController`+Window.swift around lines 11 - 14, The SuggestionPanel type lacks explicit access control; change its declaration from "final class SuggestionPanel" to an explicit access level (e.g., "private final class SuggestionPanel") since it is only used internally by SuggestionController, and update any references in SuggestionController to match that access level so the compiler and style guidelines are satisfied.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@TablePro/Views/Editor/SQLEditorView.swift`:
- Around line 82-86: The favorites binding remains tied to the old connection
because setupFavoritesObserver() captures connectionId once; when connectionId
changes (the same place where
completionAdapter.updateSchemaProvider(schemaProvider, databaseType:
databaseType) is called), tear down or re-register the favorites observer for
the new connectionId by calling the same logic as setupFavoritesObserver() (or
provide a teardown + setup sequence) so favorites suggestions are rebound to the
new connection; locate the connection change block (onChange of connectionId),
the setupFavoritesObserver() function, and the
completionAdapter.updateSchemaProvider(...) call and ensure you reinitialize the
favorites observer (or remove old observer) there when connectionId changes.
---
Nitpick comments:
In
`@LocalPackages/CodeEditSourceEditor/Sources/CodeEditSourceEditor/CodeSuggestion/Model/SuggestionViewModel.swift`:
- Line 13: The logger subsystem string in the static logger declaration uses
"com.TablePro" which is package-mismatched; update the Logger initialization in
SuggestionViewModel (the private static let logger) to use a package-appropriate
subsystem such as "com.CodeEditSourceEditor" (or another package-specific
identifier) so logs are correctly categorized for this package while keeping the
category "SuggestionVM" unchanged.
In
`@LocalPackages/CodeEditSourceEditor/Sources/CodeEditSourceEditor/CodeSuggestion/Window/SuggestionController`+Window.swift:
- Around line 11-14: The SuggestionPanel type lacks explicit access control;
change its declaration from "final class SuggestionPanel" to an explicit access
level (e.g., "private final class SuggestionPanel") since it is only used
internally by SuggestionController, and update any references in
SuggestionController to match that access level so the compiler and style
guidelines are satisfied.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7f01c22e-7b52-4cb4-8e1c-ef1c4e439bea
📒 Files selected for processing (12)
LocalPackages/CodeEditSourceEditor/Sources/CodeEditSourceEditor/CodeSuggestion/Model/CodeSuggestionDelegate.swiftLocalPackages/CodeEditSourceEditor/Sources/CodeEditSourceEditor/CodeSuggestion/Model/SuggestionTriggerCharacterModel.swiftLocalPackages/CodeEditSourceEditor/Sources/CodeEditSourceEditor/CodeSuggestion/Model/SuggestionViewModel.swiftLocalPackages/CodeEditSourceEditor/Sources/CodeEditSourceEditor/CodeSuggestion/Window/SuggestionController+Window.swiftLocalPackages/CodeEditSourceEditor/Sources/CodeEditSourceEditor/CodeSuggestion/Window/SuggestionController.swiftLocalPackages/CodeEditSourceEditor/Sources/CodeEditSourceEditor/Controller/TextViewController+Lifecycle.swiftLocalPackages/CodeEditSourceEditor/Sources/CodeEditSourceEditor/SourceEditor/SourceEditor.swiftTablePro/Core/Autocomplete/CompletionEngine.swiftTablePro/Core/Autocomplete/SQLCompletionProvider.swiftTablePro/Core/Autocomplete/SQLSchemaProvider.swiftTablePro/Views/Editor/SQLCompletionAdapter.swiftTablePro/Views/Editor/SQLEditorView.swift
- Add setupFavoritesObserver() on connectionId change to rebind favorites - Add explicit access control to SuggestionPanel (internal) - Use package-appropriate logger subsystem in CodeEditSourceEditor
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
weak vardelegate references onSourceEditorstruct with strong vars (TextViewController already keeps proper weak refs)NSWindowwithNSPanelsubclass (SuggestionPanel) to fixcanBecomeKeyWindowwarningsisManualTriggerflag so Ctrl+Space always shows completions even with empty prefix/empty editorupdateSchemaProvidercode on connection changeos.Logger) at key failure points in the completion pipelineTest plan
SELECT * FROM u— autocomplete appears atuSELECTrapidly — completions appear after last keystroke (no dropped triggers)canBecomeKeyWindowwarningsSummary by CodeRabbit
New Features
Bug Fixes
Chores