feat: copy rows as INSERT/UPDATE SQL statements#271
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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)
📝 WalkthroughWalkthroughAdds "Copy as INSERT/UPDATE" support: a new SQLRowToStatementConverter generates DB-specific INSERT/UPDATE SQL, UI wiring in DataGridView and context menu, coordinator clipboard actions, new localization keys, and comprehensive unit tests. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Menu as TableRowViewWithMenu
participant Coordinator as TableViewCoordinator
participant Converter as SQLRowToStatementConverter
participant Clipboard
User->>Menu: Right-click row(s)
Menu->>Menu: Display context menu with "Copy as"
User->>Menu: Select "INSERT Statement(s)" / "UPDATE Statement(s)"
Menu->>Coordinator: copyRowsAsInsert(at: indices) / copyRowsAsUpdate(at: indices)
Coordinator->>Coordinator: Gather rows, tableName, primaryKeyColumn, databaseType
Coordinator->>Converter: generateInserts(rows) / generateUpdates(rows)
Converter->>Converter: Quote identifiers, format values, build SQL per DB
Converter-->>Coordinator: Return SQL string
Coordinator->>Clipboard: Write SQL string to clipboard
Clipboard-->>User: SQL ready to paste
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 unit tests (beta)
Comment |
Signed-off-by: Ngô Quốc Đạt <datlechin@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
TablePro/Core/Utilities/SQL/SQLRowToStatementConverter.swift (1)
7-13: Consider adding explicit access control.Per coding guidelines: "Always specify access control explicitly on both extensions and individual members." The struct and its public-facing methods (
generateInserts,generateUpdates) lack explicit modifiers.Suggested change
-struct SQLRowToStatementConverter { - let tableName: String +internal struct SQLRowToStatementConverter { + internal let tableName: String(Apply similar pattern to other members)
As per coding guidelines: "Always specify access control explicitly."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Core/Utilities/SQL/SQLRowToStatementConverter.swift` around lines 7 - 13, The struct SQLRowToStatementConverter and its members need explicit access control; update the declaration of SQLRowToStatementConverter and add explicit access modifiers to its public-facing methods generateInserts and generateUpdates (and other members like tableName, columns, primaryKeyColumn, databaseType, and maxRows) to match the module's API surface (e.g., public/internal/private as appropriate) so access levels are explicit rather than implicit.
🤖 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/Core/Utilities/SQL/SQLRowToStatementConverter.swift`:
- Line 52: The WHERE clause in SQLRowToStatementConverter currently builds
"quoteColumn(pkColumn) = formatValue(pkValue)" which yields "= NULL" when
pkValue is nil; change the logic that sets whereClause so it checks pkValue (or
the underlying optional) and uses "quoteColumn(pkColumn) IS NULL" when pkValue
is nil, otherwise keep the existing "quoteColumn(pkColumn) =
formatValue(pkValue)" behavior; update the code around whereClause assignment
(reference symbols: whereClause, pkColumn, pkValue, quoteColumn(_:),
formatValue(_:)) to branch on nil and produce the correct IS NULL SQL syntax.
- Around line 78-83: The formatValue(_:) function currently only doubles single
quotes and returns the literal "NULL" for nil, which misses backslash escaping
and leads to invalid WHERE clauses for NULL PKs; update formatValue(_ value:
String?) to also escape backslashes by replacing "\" with "\\", continue to
escape single quotes, and still return "NULL" for nil, and then modify the
UPDATE/WHERE construction that uses pkValue so it emits "col IS NULL" when
pkValue is nil instead of "col = NULL" (use formatValue only for non-nil
pkValue); reference formatValue(_:) and the code path that builds the WHERE
clause from pkValue to implement these two fixes.
In `@TablePro/Resources/Localizable.xcstrings`:
- Around line 4067-4070: The "Copy as" localization entries (e.g., the "Copy as"
key in Localizable.xcstrings and the similar empty entries at ranges 8456-8459
and 16645-16647) are missing translations; update these keys for vi and zh-Hans
with appropriate localized strings for the surrounding UI while preserving SQL
keywords like INSERT and UPDATE verbatim (do not translate font names, database
types, encodings, or other technical terms). Locate the empty dictionary values
for "Copy as" and the other empty submenu labels in Localizable.xcstrings, add
the translated phrases for Vietnamese and Simplified Chinese, and ensure any
occurrences of INSERT/UPDATE remain uppercase and unchanged.
---
Nitpick comments:
In `@TablePro/Core/Utilities/SQL/SQLRowToStatementConverter.swift`:
- Around line 7-13: The struct SQLRowToStatementConverter and its members need
explicit access control; update the declaration of SQLRowToStatementConverter
and add explicit access modifiers to its public-facing methods generateInserts
and generateUpdates (and other members like tableName, columns,
primaryKeyColumn, databaseType, and maxRows) to match the module's API surface
(e.g., public/internal/private as appropriate) so access levels are explicit
rather than implicit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5b2415ce-5377-400f-8b67-6a4309563fc5
📒 Files selected for processing (8)
CHANGELOG.mdTablePro/Core/Utilities/SQL/SQLRowToStatementConverter.swiftTablePro/Resources/Localizable.xcstringsTablePro/Views/Main/Child/MainEditorContentView.swiftTablePro/Views/Results/DataGridView+RowActions.swiftTablePro/Views/Results/DataGridView.swiftTablePro/Views/Results/TableRowViewWithMenu.swiftTableProTests/Core/Utilities/SQLRowToStatementConverterTests.swift
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
TablePro/Core/Utilities/SQL/SQLRowToStatementConverter.swift (1)
82-87:⚠️ Potential issue | 🔴 CriticalMySQL/MariaDB string literals still need backslash escaping.
This still only doubles
'. For MySQL/MariaDB, values containing\are not round-trippable under the default SQL mode unless backslashes are escaped too, so the copied SQL can change data or fail to parse.Suggested change
private func formatValue(_ value: String?) -> String { guard let value else { return "NULL" } - return "'\(value.replacingOccurrences(of: "'", with: "''"))'" + var escaped = value.replacingOccurrences(of: "'", with: "''") + if databaseType == .mysql || databaseType == .mariadb { + escaped = escaped.replacingOccurrences(of: "\\", with: "\\\\") + } + return "'\(escaped)'" }For MySQL and MariaDB SQL string literals, are backslashes escape characters by default, and how should a generated literal escape `\` when `NO_BACKSLASH_ESCAPES` is not enabled?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Core/Utilities/SQL/SQLRowToStatementConverter.swift` around lines 82 - 87, The formatValue(_ value: String?) function currently only doubles single quotes but must also escape backslashes for MySQL/MariaDB when NO_BACKSLASH_ESCAPES is not enabled; update formatValue to first escape backslashes (replace "\" with "\\") and then escape single quotes (replace "'" with "''"), then return the quoted literal or "NULL" as before so generated SQL round-trips correctly for MySQL/MariaDB.
🤖 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/Core/Utilities/SQL/SQLRowToStatementConverter.swift`:
- Around line 42-56: The current logic treats a missing PK as "IS NULL" which
can match unintended rows; change the PK handling so you only build the pk WHERE
clause when the pk index exists in columns and the row actually contains a value
for that index (non-nil). Use primaryKeyColumn, pkIndex, pkValue, whereClause,
quoteColumn and formatValue to: compute pkIndex, check
row.indices.contains(pkIndex) and that the extracted pkValue is non-nil before
setting whereClause to either "= <value>" or "IS NULL" only if the value is
explicitly null; otherwise, when the PK is unavailable, do not emit "IS NULL" —
instead make whereClause a safe no-match condition (e.g. "1=0") or bail out so
you don't update unintended rows.
- Around line 7-31: Mark the new type and its non-private members with explicit
access control: declare the struct SQLRowToStatementConverter as internal, add
explicit internal to the stored properties tableName, columns, primaryKeyColumn,
and databaseType, and to the non-private methods generateInserts(rows:),
generateUpdates(rows:) and any other non-private helpers like
buildUpdateStatement(...); keep the existing private modifier on maxRows as-is.
Ensure every member has an explicit access level (private/internal/public as
appropriate) to satisfy the repository convention.
---
Duplicate comments:
In `@TablePro/Core/Utilities/SQL/SQLRowToStatementConverter.swift`:
- Around line 82-87: The formatValue(_ value: String?) function currently only
doubles single quotes but must also escape backslashes for MySQL/MariaDB when
NO_BACKSLASH_ESCAPES is not enabled; update formatValue to first escape
backslashes (replace "\" with "\\") and then escape single quotes (replace "'"
with "''"), then return the quoted literal or "NULL" as before so generated SQL
round-trips correctly for MySQL/MariaDB.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bfccc20d-791b-40b2-901b-a78dc293d067
📒 Files selected for processing (2)
TablePro/Core/Utilities/SQL/SQLRowToStatementConverter.swiftTableProTests/Core/Utilities/SQLRowToStatementConverterTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- TableProTests/Core/Utilities/SQLRowToStatementConverterTests.swift
This was referenced Mar 12, 2026
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
SQLRowToStatementConvertergenerates fully-inlined SQL from selected row data, with proper identifier quoting per database type and value escapingALTER TABLE ... UPDATEsyntax)Closes #269
Test plan
SQLRowToStatementConverterTests)Summary by CodeRabbit