feat: PK metadata detection, save preserves original query, AI Assistant persistence#215
Open
kix007 wants to merge 2 commits into
Open
feat: PK metadata detection, save preserves original query, AI Assistant persistence#215kix007 wants to merge 2 commits into
kix007 wants to merge 2 commits into
Conversation
…ant persistence - PK detection now queries real DB metadata from information_schema.table_constraints (PostgreSQL/MySQL) and PRAGMA table_info (SQLite) instead of relying solely on name heuristics - Unified getPrimaryKey helper used across all CRUD operations - Save no longer replaces custom query results with SELECT * FROM table — re-runs the user's original SELECT via lastSelectQueryRef - Schema-qualified table PK candidates use base table name (config_id not ansible.config_id) - Fallback UPDATE WHERE uses _original snapshot to prevent 0-row updates - Grid scrolls to saved/inserted record after save - AI Assistant: extracted shared callAI to src/lib/ai.ts, wired AIAssistantDialog to real provider (no longer setTimeout stub), added AI settings persistence to settingsStore - Roadmap: removed shipped items (vault hardening, AI assistant, schema-aware row editor, ER diagram)
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR enhances QueryDen with primary key metadata discovery across PostgreSQL, MySQL, and SQLite, refactors row editing to preserve original queries and scroll to changes, prevents execution race conditions, and wires a real AI Assistant dialog with settings persistence. ChangesRow Editing and Execution Flow Improvements
AI Assistant Implementation and Settings
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/layout/MainContent.tsx (1)
2554-2572:⚠️ Potential issue | 🟠 Major | ⚡ Quick winProvider-specific branching uses the wrong connection context.
These branches use
activeConnection.type, but save/add now execute on resolvedconn(tab target can differ). If types differ, wrong SQL path is chosen.Proposed fix
- if (["postgres", "supabase", "cockroach", "mysql", "mariadb"].includes(activeConnection.type)) { + if (["postgres", "supabase", "cockroach", "mysql", "mariadb"].includes(conn.type)) { ... - } else if (activeConnection.type === "sqlite") { + } else if (conn.type === "sqlite") { ... - if (activeConnection.type === "mysql" || activeConnection.type === "mariadb") { + if (conn.type === "mysql" || conn.type === "mariadb") {Also applies to: 2785-2786
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/layout/MainContent.tsx` around lines 2554 - 2572, The provider-specific branching logic is checking activeConnection.type to determine which SQL path to use, but the database operations are being executed on the resolved conn object (which may differ from activeConnection if targeting a different tab). Replace the activeConnection.type checks in the conditional statements (at the lines checking for postgres, supabase, cockroach, mysql, mariadb, and sqlite) with checks against the actual resolved connection type that is being used for the database operations, ensuring the correct SQL dialect is selected based on the connection being executed rather than the currently active connection.
🧹 Nitpick comments (2)
src/components/tools/AIAssistantDialog.tsx (1)
30-58: ⚖️ Poor tradeoffConsider UX: Dialog closes immediately after success.
Line 52 closes the dialog immediately after updating the query. This means users don't see any success confirmation or the AI response before the dialog disappears. Consider showing a brief success state or allowing users to review the generated SQL before closing.
💡 Optional UX improvement
Add a success state and delay closing:
const result = await callAI(systemPrompt, userPrompt); if (result) { onUpdateQuery(result); + // Show success briefly before closing + setTimeout(() => onClose(), 500); + } else { + onClose(); } - onClose();Or allow users to review before accepting:
- Add a
generatedSQLstate- Show the generated SQL in a preview area
- Add "Accept" and "Cancel" buttons
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/tools/AIAssistantDialog.tsx` around lines 30 - 58, handleGenerate currently calls onUpdateQuery(result) and onClose() immediately, closing the dialog before the user can see or confirm the AI output; change this to save the AI response into a new generatedSQL state (setGeneratedSQL(result)) instead of immediately calling onClose, remove the immediate onUpdateQuery/onClose, and surface a preview in the dialog with "Accept" and "Cancel" actions where Accept calls onUpdateQuery(generatedSQL) then onClose and Cancel clears generatedSQL; alternatively implement a brief success state/toast and a short delay before calling onClose if an explicit review step is undesired.src/lib/ai.ts (1)
76-86: ⚡ Quick winSilent failure on response parsing.
Lines 76-85 use optional chaining to extract the AI response, returning an empty string
""if parsing fails (line 85). This silently swallows malformed responses, making debugging difficult. Consider logging a warning or throwing an error when the expected response shape is missing.🛠️ Suggested improvement
if (ai.provider === "openai") { - return result.choices?.[0]?.message?.content || ""; + const content = result.choices?.[0]?.message?.content; + if (!content) { + console.warn("[callAI] OpenAI response missing expected content:", result); + } + return content || ""; } else if (ai.provider === "anthropic") { - return result.content?.[0]?.text || ""; + const text = result.content?.[0]?.text; + if (!text) { + console.warn("[callAI] Anthropic response missing expected text:", result); + } + return text || ""; } else if (ai.provider === "google") { - return result.candidates?.[0]?.content?.parts?.[0]?.text || ""; + const text = result.candidates?.[0]?.content?.parts?.[0]?.text; + if (!text) { + console.warn("[callAI] Google response missing expected text:", result); + } + return text || ""; } else if (ai.provider === "local") { - return result.message?.content || ""; + const content = result.message?.content; + if (!content) { + console.warn("[callAI] Local AI response missing expected content:", result); + } + return content || ""; } - return ""; + console.warn("[callAI] Unknown provider, returning empty response"); + return "";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/ai.ts` around lines 76 - 86, The response-parsing block that reads ai.provider and returns parts of result currently falls back to an empty string on malformed responses, which hides errors; update the parsing logic (the branch that checks ai.provider and reads result) to detect when the expected shape is missing and either log a warning including the provider and the full result (e.g., JSON.stringify(result)) or throw a descriptive error mentioning ai.provider and the unexpected result shape instead of silently returning "". Ensure you reference the same identifiers (ai.provider and result) so the fix applies to the existing branches for "openai", "anthropic", "google", and "local".
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 72-73: There are two identical "### Removed" headings under the
"Unreleased" section causing MD024; locate the duplicate "### Removed" heading
(the second occurrence) and either remove it or merge its bullet(s) into the
first "### Removed" list so only one "### Removed" heading remains; ensure the
bullets mentioning "Vault crypto hardening", "Schema-aware row editor", and "ER
Diagram" are preserved under the single heading.
In `@src/components/layout/MainContent.tsx`:
- Around line 2039-2043: The finally block currently clears
cancelFlagRef.current unconditionally which can clear a newer run's cancel
state; change the cleanup to only run when the finished execution generation
matches the active one by wrapping the cancelFlagRef.current = false (and the
existing setIsExecuting(false)/isExecutingRef.current = false) in the same guard
using gen === executionGenRef.current so only the matching generation resets the
cancel flag and execution state.
- Around line 2365-2373: getPrimaryKey currently returns only the first matching
column which collapses composite primary keys; change the function (e.g., rename
to getPrimaryKeys) to return string[] | undefined and collect all matching PK
columns instead of the first: (1) if tableSchema?.primaryKeys exists return all
primaryKeys that are present in the provided columns array; (2) otherwise build
the candidate set (including `${baseTableName.toLowerCase()}_id`) and return all
columns that case-insensitively match any candidate; (3) update all callers that
relied on a single PK to accept multiple PK columns and construct WHERE/matching
clauses by joining those PK conditions with AND so updates/deletes correctly
target composite-key rows.
In `@src/store/aiStore.ts`:
- Line 21: The default AI model value is invalid ("gpt-5.5"); update the `model`
property in the aiStore default settings to a valid OpenAI model name (for
example "gpt-4o", "gpt-4-turbo", or "gpt-3.5-turbo") and make the same change
where the same default appears in settingsStore (the mirrored `model` property
referenced in settingsStore.ts) so both defaults match a supported model.
---
Outside diff comments:
In `@src/components/layout/MainContent.tsx`:
- Around line 2554-2572: The provider-specific branching logic is checking
activeConnection.type to determine which SQL path to use, but the database
operations are being executed on the resolved conn object (which may differ from
activeConnection if targeting a different tab). Replace the
activeConnection.type checks in the conditional statements (at the lines
checking for postgres, supabase, cockroach, mysql, mariadb, and sqlite) with
checks against the actual resolved connection type that is being used for the
database operations, ensuring the correct SQL dialect is selected based on the
connection being executed rather than the currently active connection.
---
Nitpick comments:
In `@src/components/tools/AIAssistantDialog.tsx`:
- Around line 30-58: handleGenerate currently calls onUpdateQuery(result) and
onClose() immediately, closing the dialog before the user can see or confirm the
AI output; change this to save the AI response into a new generatedSQL state
(setGeneratedSQL(result)) instead of immediately calling onClose, remove the
immediate onUpdateQuery/onClose, and surface a preview in the dialog with
"Accept" and "Cancel" actions where Accept calls onUpdateQuery(generatedSQL)
then onClose and Cancel clears generatedSQL; alternatively implement a brief
success state/toast and a short delay before calling onClose if an explicit
review step is undesired.
In `@src/lib/ai.ts`:
- Around line 76-86: The response-parsing block that reads ai.provider and
returns parts of result currently falls back to an empty string on malformed
responses, which hides errors; update the parsing logic (the branch that checks
ai.provider and reads result) to detect when the expected shape is missing and
either log a warning including the provider and the full result (e.g.,
JSON.stringify(result)) or throw a descriptive error mentioning ai.provider and
the unexpected result shape instead of silently returning "". Ensure you
reference the same identifiers (ai.provider and result) so the fix applies to
the existing branches for "openai", "anthropic", "google", and "local".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fb16765a-0c4e-4941-bd2e-e316dffc533e
📒 Files selected for processing (12)
CHANGELOG.mdsrc/components/explorer/DatabaseExplorer.tsxsrc/components/layout/MainContent.tsxsrc/components/results/ResultsPanel.tsxsrc/components/results/VisualOptimizer.tsxsrc/components/settings/SettingsDialog.tsxsrc/components/tools/AIAssistantDialog.tsxsrc/lib/ai.tssrc/store/aiStore.tssrc/store/settingsStore.tswebsite/src/content/docs/ai/byo-key.mdxwebsite/src/pages/roadmap.astro
💤 Files with no reviewable changes (1)
- website/src/pages/roadmap.astro
Comment on lines
+72
to
+73
| ### Removed | ||
| - **Roadmap items removed as addressed.** "Vault crypto hardening" (#14, #15), "Schema-aware row editor" (#50), and "ER Diagram" (#51) removed from the website roadmap — vault hardening, schema-aware FK dropdowns, NOT NULL validation, and ER diagram support in this release subsume those feature requests. |
There was a problem hiding this comment.
Duplicate heading level under Unreleased (### Removed).
Line 72 introduces a second ### Removed heading in the same section, which triggers MD024 and can break docs tooling/navigation.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 72-72: Multiple headings with the same content
(MD024, no-duplicate-heading)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CHANGELOG.md` around lines 72 - 73, There are two identical "### Removed"
headings under the "Unreleased" section causing MD024; locate the duplicate "###
Removed" heading (the second occurrence) and either remove it or merge its
bullet(s) into the first "### Removed" list so only one "### Removed" heading
remains; ensure the bullets mentioning "Vault crypto hardening", "Schema-aware
row editor", and "ER Diagram" are preserved under the single heading.
Comment on lines
+2039
to
2043
| cancelFlagRef.current = false; | ||
| if (gen === executionGenRef.current) { | ||
| setIsExecuting(false); | ||
| isExecutingRef.current = false; | ||
| isExecutingRef.current = false; | ||
| } |
There was a problem hiding this comment.
Guard cancel-state cleanup to the active execution generation.
On Line 2039, cancelFlagRef.current is reset unconditionally in finally. A stale execution can clear cancel state for a newer in-flight run, causing cancel to be ignored intermittently.
Proposed fix
- cancelFlagRef.current = false;
- if (gen === executionGenRef.current) {
+ if (gen === executionGenRef.current) {
+ cancelFlagRef.current = false;
setIsExecuting(false);
isExecutingRef.current = false;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| cancelFlagRef.current = false; | |
| if (gen === executionGenRef.current) { | |
| setIsExecuting(false); | |
| isExecutingRef.current = false; | |
| isExecutingRef.current = false; | |
| } | |
| if (gen === executionGenRef.current) { | |
| cancelFlagRef.current = false; | |
| setIsExecuting(false); | |
| isExecutingRef.current = false; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/layout/MainContent.tsx` around lines 2039 - 2043, The finally
block currently clears cancelFlagRef.current unconditionally which can clear a
newer run's cancel state; change the cleanup to only run when the finished
execution generation matches the active one by wrapping the
cancelFlagRef.current = false (and the existing
setIsExecuting(false)/isExecutingRef.current = false) in the same guard using
gen === executionGenRef.current so only the matching generation resets the
cancel flag and execution state.
Comment on lines
+2365
to
+2373
| const getPrimaryKey = useCallback((columns: string[]): string | undefined => { | ||
| if (tableSchema?.primaryKeys && tableSchema.primaryKeys.length > 0) { | ||
| const found = tableSchema.primaryKeys.find(pk => columns.includes(pk)); | ||
| if (found) return found; | ||
| } | ||
| if (!activeTableName) return undefined; | ||
| const baseTableName = activeTableName.includes('.') ? activeTableName.split('.').pop()! : activeTableName; | ||
| const candidates = new Set(["id", "uuid", "uid", "pk", "row_id", "object_id", "key", "code", "_id", `${baseTableName.toLowerCase()}_id`]); | ||
| return columns.find(c => candidates.has(c.toLowerCase())); |
There was a problem hiding this comment.
Composite primary keys are collapsed to one column.
getPrimaryKey returns only the first matching PK column. For composite PK tables, update/delete matching can target multiple rows incorrectly.
Proposed direction
-const getPrimaryKey = useCallback((columns: string[]): string | undefined => {
- if (tableSchema?.primaryKeys && tableSchema.primaryKeys.length > 0) {
- const found = tableSchema.primaryKeys.find(pk => columns.includes(pk));
- if (found) return found;
- }
+const getPrimaryKeys = useCallback((columns: string[]): string[] => {
+ if (tableSchema?.primaryKeys?.length) {
+ const found = tableSchema.primaryKeys.filter(pk => columns.includes(pk));
+ if (found.length) return found;
+ }
...
- return columns.find(c => candidates.has(c.toLowerCase()));
-}, [tableSchema, activeTableName]);
+ const heuristic = columns.find(c => candidates.has(c.toLowerCase()));
+ return heuristic ? [heuristic] : [];
+}, [tableSchema, activeTableName]);Then build WHERE/row matching using all returned PK columns with AND.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/layout/MainContent.tsx` around lines 2365 - 2373,
getPrimaryKey currently returns only the first matching column which collapses
composite primary keys; change the function (e.g., rename to getPrimaryKeys) to
return string[] | undefined and collect all matching PK columns instead of the
first: (1) if tableSchema?.primaryKeys exists return all primaryKeys that are
present in the provided columns array; (2) otherwise build the candidate set
(including `${baseTableName.toLowerCase()}_id`) and return all columns that
case-insensitively match any candidate; (3) update all callers that relied on a
single PK to accept multiple PK columns and construct WHERE/matching clauses by
joining those PK conditions with AND so updates/deletes correctly target
composite-key rows.
| provider: "openai", | ||
| apiKey: "", | ||
| model: "gpt-4o", | ||
| model: "gpt-5.5", |
There was a problem hiding this comment.
Invalid default AI model name.
The default model is set to "gpt-5.5", which does not match any known OpenAI model. This mirrors the same issue in settingsStore.ts line 155. The value should be updated to a valid model such as "gpt-4o", "gpt-4-turbo", or "gpt-3.5-turbo".
🛠️ Suggested fix
- model: "gpt-5.5",
+ model: "gpt-4o",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| model: "gpt-5.5", | |
| model: "gpt-4o", |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/store/aiStore.ts` at line 21, The default AI model value is invalid
("gpt-5.5"); update the `model` property in the aiStore default settings to a
valid OpenAI model name (for example "gpt-4o", "gpt-4-turbo", or
"gpt-3.5-turbo") and make the same change where the same default appears in
settingsStore (the mirrored `model` property referenced in settingsStore.ts) so
both defaults match a supported model.
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
2 participants
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
Related issue
Changes
Testing notes
Checklist
npm run tauri devornpm run tauri build) and verified the change works.npx tsc --noEmit).cargo check --manifest-path src-tauri/Cargo.toml).#[tauri::command], I registered it insrc-tauri/src/lib.rs'sgenerate_handler!.CHANGELOG.mdunder## [Unreleased].storage.rs,updater.rs,capabilities/, or the AI assistant code path, I flagged it in the summary above.Summary by CodeRabbit
New Features
Bug Fixes
Documentation