-
Notifications
You must be signed in to change notification settings - Fork 2
fix: Fix autocomplete table name issues. #25
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
Conversation
📝 WalkthroughWalkthroughThe PR changes table-name completion behavior. createTableCandidates now accumulates multiple Identifier candidates per table (always adding an unqualified name when qualificationLevel is 0) and conditionally adds database/catalog-qualified forms instead of returning early. complete.ts detects when the cursor is inside a FROM-clause table reference and emits table candidates immediately, and it removes duplicated recomputation of fromNodes/subqueryTables/schemaAndSubqueries. Tests were updated to expect matches for partial unqualified table names and add cases covering database-qualified names and partial-parse scenarios. Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
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: 2
📜 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 (4)
packages/server/src/complete/candidates/createTableCandidates.ts(4 hunks)packages/server/src/complete/complete.ts(1 hunks)packages/server/test/complete.test.ts(1 hunks)packages/server/test/complete/complete_table.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
packages/server/test/complete/complete_table.test.ts (2)
packages/server/src/complete/complete.ts (2)
complete(79-134)complete(486-499)packages/server/src/complete/index.ts (1)
complete(4-4)
packages/server/test/complete.test.ts (1)
packages/server/src/complete/complete.ts (2)
complete(79-134)complete(486-499)
packages/server/src/complete/candidates/createTableCandidates.ts (2)
packages/server/src/complete/Identifier.ts (1)
Identifier(5-81)packages/server/src/complete/CompletionItemUtils.ts (1)
ICONS(4-13)
🔇 Additional comments (7)
packages/server/src/complete/candidates/createTableCandidates.ts (3)
30-44: Good addition for unqualified table matching.This enables typing "act" to match "actor" even when tables have database qualifiers. Clean implementation.
56-68: Correct guard against duplicate suggestions.The
qualificationLevel > 0check properly avoids adding the fully qualified name when it would duplicate the table-name-only entry added above.
84-101: Accumulation pattern looks good.Switching from early returns to
results.push()allows multiple candidate forms per table. Cleaner control flow.packages/server/test/complete.test.ts (1)
556-569: Test validates issue #24 fix.Clear assertions for partial matching. The comment referencing the issue is helpful for future maintainers.
packages/server/src/complete/complete.ts (1)
362-380: Good handling of successfully-parsed table name typing.This catches the edge case where partial table names parse as valid tables. The case-insensitive check and early return prevent column pollution.
packages/server/test/complete/complete_table.test.ts (2)
153-185: Solid coverage for database-qualified tables.Tests the core fix: unqualified partial input matching qualified tables.
210-259: Good multi-table filtering test.Validates prefix matching with exclusions. Covers
film*matching while excludingactorandcustomer.
saltenasl
left a comment
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.
LGTM, thanks!
| const schema = { | ||
| tables: [ | ||
| { | ||
| catalog: null, | ||
| database: 'squeal', | ||
| tableName: 'actor', | ||
| columns: [], | ||
| }, | ||
| { | ||
| catalog: null, | ||
| database: 'squeal', | ||
| tableName: 'actor_info', | ||
| columns: [], | ||
| }, | ||
| { | ||
| catalog: null, | ||
| database: 'squeal', | ||
| tableName: 'film', | ||
| columns: [], | ||
| }, | ||
| { | ||
| catalog: null, | ||
| database: 'squeal', | ||
| tableName: 'film_actor', | ||
| columns: [], | ||
| }, | ||
| { | ||
| catalog: null, | ||
| database: 'squeal', | ||
| tableName: 'customer', | ||
| columns: [], | ||
| }, | ||
| ], | ||
| functions: [], | ||
| } |
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.
nit - i'd create a helper function for creating schema, something along the lines of:
schema(
table('actor'),
table('actor_info'),
// ...
)
it being so sparse makes it a bit hard to understand at a glance
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.
That's a good point
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: 2
📜 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(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/server/test/complete/complete_table.test.ts (1)
packages/server/src/complete/complete.ts (2)
complete(79-134)complete(488-501)
🔇 Additional comments (1)
packages/server/test/complete/complete_table.test.ts (1)
3-25: simpleSchema fixture + updated usages look consistentThe in-file
simpleSchemakeeps the previous structure and the updated call sites all pass it as before, so behavior of these existing tests should be unchanged while keeping the naming consistent with the new helpers.Also applies to: 113-187
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Performance
✏️ Tip: You can customize this high-level summary in your review settings.