-
Notifications
You must be signed in to change notification settings - Fork 2
fix: Don't include columns when completing FROM statements. #22
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
fix: Don't include columns when completing FROM statements. #22
Conversation
📝 WalkthroughWalkthroughThe PR changes SELECT query completion so that when the cursor is typing a table name in a FROM/JOIN context, scoped and unscoped column candidates are not generated; aliases remain suggested. Detection of "after FROM" was broadened to recognize JOIN patterns (INNER/LEFT/RIGHT/FULL, etc.). Control flow reordered: aliases are added first, then column candidates only when not typing a table name. A test verifies that a partial table name after FROM yields table-name suggestions and excludes column-name suggestions. Sequence Diagram(s)sequenceDiagram
actor Editor
participant CompletionEngine
participant SchemaStore
Editor->>CompletionEngine: request completions (query, cursorPos)
CompletionEngine->>CompletionEngine: analyze token/context (isTypingTableName?)
alt isTypingTableName == true
CompletionEngine->>SchemaStore: fetch matching table names
SchemaStore-->>CompletionEngine: table candidates
CompletionEngine-->>Editor: return table candidates (+ aliases if applicable)
else isTypingTableName == false
CompletionEngine->>SchemaStore: fetch columns (scoped & unscoped) and aliases
SchemaStore-->>CompletionEngine: column candidates
CompletionEngine-->>Editor: return aliases + column candidates
end
Pre-merge checks✅ Passed checks (3 passed)
Comment |
009c14b to
ca698f1
Compare
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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/server/src/complete/complete.ts (1)
258-291: Excellent. I have completed the verification. The review comment is accurate and the issue is confirmed. Let me now output the final rewritten review:
Current JOIN regex misses
LEFT OUTER JOINandRIGHT OUTER JOINpatternsThe
isTypingTableNamegating correctly prevents column candidates from showing up when the cursor is in the FROM/JOIN area while still surfacing aliases and table names.However, the JOIN regex on line 269 has a real gap:
/^(INNER |LEFT |RIGHT |FULL |FULL OUTER |CROSS |NATURAL |OUTER )?JOIN( |$)/This pattern does not match
LEFT OUTER JOINorRIGHT OUTER JOIN. The regex requires the space to be part of the alternative (e.g.,LEFTexpectsJOINimmediately after), but withLEFT OUTER, the next token isOUTER, notJOIN. OnlyFULL OUTER JOINis covered because it's explicitly listed as a single alternative.When users type after
LEFT OUTER JOINorRIGHT OUTER JOIN,isCursorAfterFromKeywordremainsfalse, so column suggestions will still appear instead of table names—defeating the purpose of this change.A more robust pattern:
- /^(INNER |LEFT |RIGHT |FULL |FULL OUTER |CROSS |NATURAL |OUTER )?JOIN( |$)/.test( - afterFromClause - ) + /^(?:(?:INNER|LEFT|RIGHT|FULL|CROSS|NATURAL|OUTER)(?:\s+OUTER)?\s+)?JOIN(?:\s|$)/.test( + afterFromClause + )This covers
JOIN,INNER JOIN,LEFT JOIN,LEFT OUTER JOIN,RIGHT OUTER JOIN,FULL OUTER JOIN, etc., after the.trim().toUpperCase()already applied.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/server/src/complete/complete.ts(2 hunks)packages/server/test/complete/complete_table.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/server/test/complete/complete_table.test.ts (1)
packages/server/src/complete/complete.ts (2)
complete(79-134)complete(461-474)
packages/server/src/complete/complete.ts (1)
packages/server/src/complete/AstUtils.ts (1)
isPosInLocation(35-42)
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.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/server/test/complete/complete_table.test.ts(1 hunks)
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.