Refactor: Declarative ESC key handling system with security fixes#23
Merged
Refactor: Declarative ESC key handling system with security fixes#23
Conversation
- Add new declarative ESC key handling system with priority-based coordination - Core/KeyboardHandling/EscapeKeyHandler.swift - Handler types and priorities - Core/KeyboardHandling/EscapeKeyEnvironment.swift - SwiftUI environment integration - Core/KeyboardHandling/EscapeKeyCoordinator.swift - Global NSEvent monitor - Core/KeyboardHandling/View+EscapeKey.swift - Declarative SwiftUI API - Core/KeyboardHandling/EscapeKeyEnvironmentBridge.swift - Environment bridge - Migrate all sheets and views to new .escapeKeyHandler() API - Remove all .keyboardShortcut(.escape) and .keyboardShortcut(.cancelAction) - Remove manual NSEvent monitors from ContentView and DatabaseSwitcherSheetV2 - Remove .onExitCommand handlers in favor of declarative handlers - Clean up global state tracking - Remove AppState.isSheetPresented tracking - Remove isDatabaseSwitcherOpen FocusedValue - Remove unnecessary conditional checks - Add database metadata and creation features - Add DatabaseMetadata model with table count, size, last accessed - Add fetchDatabaseMetadata() to DatabaseDriver protocol - Add createDatabase() to DatabaseDriver protocol - Implement metadata fetching for MySQL, PostgreSQL, SQLite drivers - Add DatabaseSwitcherSheetV2 with rich metadata display - Add CreateDatabaseSheet for creating new databases - Fix critical security vulnerabilities - Fix SQL injection in MySQLDriver.fetchDatabaseMetadata() - Fix SQL injection in MySQLDriver.createDatabase() - Fix SQL injection in PostgreSQLDriver.fetchDatabaseMetadata() - Fix SQL injection in PostgreSQLDriver.createDatabase() - Add input validation and escaping for database names and parameters - Update menu commands to work with new ESC key system - Remove manual state checks in TableProApp commands - Install global ESC key handling via .escapeKeySystem() Build status: Verified successful compilation
There was a problem hiding this comment.
Pull request overview
This PR refactors ESC key handling from manual NSEvent monitoring to a declarative, SwiftUI environment-based system with priority coordination. It also introduces database metadata features and fixes SQL injection vulnerabilities in database creation operations.
Changes:
- Implemented declarative ESC key handling system with priority-based coordination (
.popup>.nestedSheet>.sheet>.view>.global) - Migrated all sheets and dialogs to use new
.escapeKeyHandler()and.escapeKeyDismiss()APIs - Added database metadata features including table count, size tracking, and recent database tracking
- Fixed SQL injection vulnerabilities in MySQL and PostgreSQL database creation and metadata fetching
Reviewed changes
Copilot reviewed 31 out of 32 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| Core/KeyboardHandling/* | New declarative ESC key handling system with environment-based coordination |
| Views/DatabaseSwitcher/DatabaseSwitcherSheetV2.swift | Redesigned database switcher with metadata display and create database functionality |
| Views/DatabaseSwitcher/CreateDatabaseSheet.swift | New sheet for creating databases with charset/collation options |
| ViewModels/DatabaseSwitcherViewModel.swift | ViewModel handling database fetching, metadata, and recent tracking |
| Models/DatabaseMetadata.swift | New metadata model with table count, size, and system database detection |
| Core/Database/*Driver.swift | Added metadata fetching and database creation with SQL injection fixes |
| TableProApp.swift | Removed manual ESC key monitoring and installed new declarative system |
| ContentView.swift | Removed manual NSEvent monitoring for ESC key |
| Multiple dialog/sheet files | Migrated to new .escapeKeyHandler() API |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Ngô Quốc Đạt <datlechin@gmail.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Ngô Quốc Đạt <datlechin@gmail.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Ngô Quốc Đạt <datlechin@gmail.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Ngô Quốc Đạt <datlechin@gmail.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Ngô Quốc Đạt <datlechin@gmail.com>
Replace custom tap gesture handling with native SwiftUI List selection: - Use List(selection:) with @published selectedDatabase binding - Apply DoubleClickView overlay pattern (from WelcomeWindowView) - DoubleClickView forwards single-clicks via super.mouseDown() for instant selection - Double-clicks trigger database opening This eliminates the 300-500ms delay from tap gesture disambiguation, making the switcher feel native and responsive like macOS Finder. Files: - Rename DatabaseSwitcherSheetV2 → DatabaseSwitcherSheet - Delete old DatabaseSwitcherSheet_OLD.swift - Update MainContentAlerts to reference new name - Add selectedDatabase @published property to ViewModel
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 30 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+118
to
+123
| .onExitCommand { | ||
| // Prevent dismissing the sheet via ESC while a database is being created | ||
| if !isCreating { | ||
| dismiss() | ||
| } | ||
| } |
There was a problem hiding this comment.
The .onExitCommand modifier is being used here, but according to the PR description, this refactor aims to replace all manual keyboard handling with the new .escapeKeyHandler() API. This sheet should use .escapeKeyHandler(priority: .nestedSheet) instead of .onExitCommand for consistency with the rest of the codebase.
| throw DatabaseError.queryFailed("Invalid character set: \(charset)") | ||
| } | ||
|
|
||
| var query = "CREATE DATABASE `\(escapedName)` CHARACTER SET \(charset)" |
There was a problem hiding this comment.
The charset parameter is validated against a whitelist but is not escaped when interpolated into the SQL query. While the validation provides some protection, it would be safer to escape the charset value or use parameterized queries to fully prevent potential SQL injection if the validation logic is ever modified or bypassed.
| guard collation.hasPrefix(charset), isSafe else { | ||
| throw DatabaseError.queryFailed("Invalid collation for charset") | ||
| } | ||
| query += " COLLATE \(collation)" |
There was a problem hiding this comment.
The collation is validated but not escaped before being interpolated into the SQL query. While character set validation is in place, adding proper escaping would provide defense in depth against potential SQL injection.
| throw DatabaseError.queryFailed("Invalid encoding: \(charset)") | ||
| } | ||
|
|
||
| var query = "CREATE DATABASE \"\(escapedName)\" ENCODING '\(normalizedCharset)'" |
There was a problem hiding this comment.
The encoding/charset is validated but not escaped when interpolated into the query. Even with validation, proper escaping would provide an additional layer of protection against SQL injection.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Ngô Quốc Đạt <datlechin@gmail.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Ngô Quốc Đạt <datlechin@gmail.com>
11 tasks
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
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
This PR refactors the ESC key handling system from manual NSEvent monitoring to a declarative, environment-based approach using SwiftUI. It also adds database metadata features and fixes critical SQL injection vulnerabilities.
Key Changes
1. Declarative ESC Key Handling System (5 new files)
.popup>.nestedSheet>.sheet>.view>.global.escapeKeyHandler(priority:) { }and.escapeKeyDismiss(priority:)2. Complete Migration (12 files updated)
.escapeKeyHandler()API.keyboardShortcut(.escape)patterns3. Database Metadata & Creation Features
DatabaseMetadatamodel with table count, size, last accessedDatabaseSwitcherSheetV2with rich metadata displayCreateDatabaseSheetfor creating databases with charset/collation4. Critical Security Fixes⚠️
fetchDatabaseMetadata()andcreateDatabase()fetchDatabaseMetadata()andcreateDatabase()Testing
Build status: ✅ BUILD SUCCEEDED
Manual Testing Checklist
Breaking Changes
None - this is an internal refactor with backward-compatible behavior.
Files Changed
New files:
Core/KeyboardHandling/(5 files)Models/DatabaseMetadata.swiftViewModels/DatabaseSwitcherViewModel.swiftViews/DatabaseSwitcher/DatabaseSwitcherSheetV2.swiftViews/DatabaseSwitcher/CreateDatabaseSheet.swiftModified files:
Deleted:
Views/DatabaseSwitcher/DatabaseSwitcherSheet.swift(replaced by V2)