Enhance database handling, GUI stability, and testing support - #112
Enhance database handling, GUI stability, and testing support#112christianhelle wants to merge 15 commits into
Conversation
📝 WalkthroughWalkthroughThis PR refactors database connection lifecycle management, hardens export operations with per-table error tracking, transforms query execution to script-based processing, adds teardown safety to the export orchestrator with pointer guards, and updates build/tooling for Qt5/Qt6 compatibility and safer memory management in result presentation. ChangesDatabase Lifecycle, Export Robustness, and Concurrency Safety
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)src/cli/script.cppIn file included from src/cli/script.cpp:1: ... [truncated 2200 characters] ... e 54, characters 4-52 src/database/dbanalyzer.cppIn file included from src/database/dbanalyzer.cpp:1: ... [truncated 1158 characters] ... install/lib/clang/18/include" src/database/inmemorydatabase.cppIn file included from src/database/inmemorydatabase.cpp:1: ... [truncated 1141 characters] ... l/lib/clang/18/include"
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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/database/dbexportdata.cpp (1)
40-43:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRecord file-open failures in
ExportDataProgress.Line 40 and Line 91 return before adding an error, but completion/reporting now depends on
hasAnyErrors()(Line 73 and Line 118). This can surface failed exports as successful upstream.Suggested fix
void DbDataExport::exportDataToSqlFile(IDatabase *database, const QString &filename, const CancellationToken *cancellationToken, ExportDataProgress *progress) const { + progress->reset(); const auto file = std::make_unique<QFile>(filename); if (!file-> open(QIODevice::WriteOnly | QIODevice::Text | QIODevice::Truncate)) { + progress->addError("export", QString("Failed to open SQL output: %1").arg(filename)); return; } - - progress->reset(); QTextStream out(file.get()); @@ void DbDataExport::exportDataToCsvFile(IDatabase *database, const QString &outputFolder, const QString &delimiter, const CancellationToken *cancellationToken, ExportDataProgress *progress) const { progress->reset(); @@ const auto file = std::make_unique<QFile>(filename); if (!file->open(QIODevice::WriteOnly | QIODevice::Text | QIODevice::Truncate)) { - return; + progress->addError(table.name, QString("Failed to open CSV output: %1").arg(filename)); + continue; }Also applies to: 73-75, 91-93, 118-120
🤖 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/database/dbexportdata.cpp` around lines 40 - 43, The code returns early when QFile::open(...) fails without recording the failure in ExportDataProgress, so upstream hasAnyErrors() may miss it; update every early-return file-open failure (the file->open(...) checks around the file write sections) to call ExportDataProgress to record the error before returning — e.g. call progress.addError() or ExportDataProgress::addError(...) with a descriptive message that includes the file name (file->fileName()) and the QFile::errorString() (or errno/text) so the failure is recorded for hasAnyErrors() and reporting.
🧹 Nitpick comments (1)
src/database/dbanalyzer.cpp (1)
25-27: ⚡ Quick winAvoid open/close churn across one analysis pass.
loadTables()now closes when it opened the DB, butloadColumns()immediately opens again in the sameanalyze()flow. Consider owning open/close once around both steps to avoid redundant connection transitions.Suggested refactor
bool DbAnalyzer::analyze(DatabaseInfo &info) const { + const bool wasOpen = this->database->isOpen(); + if (!wasOpen && !this->database->open()) { + return false; + } + const QFileInfo file(this->database->getFilename()); @@ loadTables(info); loadColumns(info); + + if (!wasOpen) { + this->database->close(); + } return true; }Also applies to: 32-33, 47-47
🤖 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/database/dbanalyzer.cpp` around lines 25 - 27, The analyze() flow currently lets loadTables() open and close the DB then loadColumns() re-opens it, causing redundant churn; modify analyze(), loadTables(), and loadColumns() so analyze() owns the open/close lifecycle: in analyze() check database->isOpen(), call database->open() if needed and remember whether you opened it, then call loadTables() and loadColumns() without them performing open/close; remove or guard the database->open()/close() calls inside loadTables() and loadColumns() (or make them conditional on not already-open) and ensure at the end of analyze() you only call database->close() if you were the opener (using the saved wasOpen flag).
🤖 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 `@build.ps1`:
- Around line 155-182: The macOS branch currently treats a missing test
executable with Write-Warning which is inconsistent with the Windows behavior;
update the macOS test-executable handling so that when Test-Path $testExe is
false it calls Write-Error with a descriptive message and then exits with exit 1
(same as Windows), ensuring $testExe, Test-Path, Write-Error and exit 1 are used
to signal a failure rather than just warning.
- Around line 112-134: The script treats a missing test executable differently
on Linux vs Windows; locate the block that checks Test-Path for $testExe (uses
Test-Path, Write-Warning, Write-Host "Executing: $testExe", and the $QtPath PATH
manipulation) and change the else branch so it mirrors the Windows behavior:
replace the Write-Warning with a Write-Error including the missing path and then
call exit 1 so missing test executables are treated as errors consistently
across platforms; preserve the surrounding PATH restore logic tied to $QtPath
and ensure $LASTEXITCODE handling remains unchanged after running the
executable.
In `@CONTEXT.md`:
- Around line 55-57: Remove ExportOrchestrator from the Seams section: edit the
Seams list to only include abstract interfaces (e.g., IDatabase) and delete the
ExportOrchestrator bullet/description (ExportOrchestrator is a concrete
orchestrator already documented under Modules). Ensure the Seams section
contains only true seams (abstract interfaces with multiple adapters) and leave
the Modules section entry for ExportOrchestrator unchanged.
In `@src/database/queryexecutor.cpp`:
- Line 15: The current assignment to QString sql uses
raw.trimmed().replace('\n', "") which removes newlines and can glue tokens;
change it to normalize whitespace instead: replace newlines and other whitespace
runs with a single space and trim the result so multiline SQL like "SELECT
...\nFROM ..." becomes "SELECT ... FROM ...". Update the expression that sets
sql (the line creating QString sql from raw) to perform a regex-based whitespace
collapse (e.g., replace sequences of whitespace characters with a single space)
and then call trimmed() so you preserve token separation while still removing
excess whitespace.
In `@src/gui/exportorchestrator.cpp`:
- Line 39: The field tearingDown_ is set to true during teardown but never
reset, causing subsequent calls to runExport to short-circuit; update the export
startup logic (e.g., at the top of runExport or wherever a new export job is
initialized) to explicitly set tearingDown_ = false before proceeding, and
ensure any early-return paths that begin a fresh export also reset tearingDown_;
check the teardown path where tearingDown_ is set true (the existing assignment)
remains but mirror it with the reset at export start so polling/completion logic
sees the correct state.
In `@src/gui/mainwindow.cpp`:
- Around line 396-401: Move the connect calls to attach ExportOrchestrator
signals before invoking export work so slots can't miss immediate
progress/completion; specifically, for calls like
exportOrchestrator->exportToSql(std::move(info), filepath, database.get()) and
exportOrchestrator->exportToCsv(...), call connect(exportOrchestrator.get(),
&ExportOrchestrator::exportProgress, this, &MainWindow::onExportProgress,
Qt::UniqueConnection) and connect(exportOrchestrator.get(),
&ExportOrchestrator::exportCompleted, this, &MainWindow::onExportCompleted,
Qt::UniqueConnection) first, then start the export; ensure you preserve the
std::move(info) semantics and apply the same reorder for the other block
(exportToCsv).
- Around line 358-364: The refresh trigger only looks for "create", "drop",
"insert", "delete" in the SQL string, so ALTER/RENAME statements don't call
analyzeDatabase(); update the conditional that examines the variable script (the
same block that calls ui->queryResultTab->setCurrentIndex(1) and
analyzeDatabase()) to also detect "alter" and "rename" (case-insensitive) so
schema-changing statements trigger ui refresh; keep the same behavior of
switching to the results tab before calling analyzeDatabase().
In `@src/gui/queryresultpresenter.cpp`:
- Around line 43-47: QueryResultPresenter::present() currently creates a
QStandardItemModel with a nullptr parent and then releases it to
tablePtr->setModel(...), causing leaked models; fix by giving the model a
QObject parent tied to the QTableView before releasing ownership (either
construct the QStandardItemModel with tablePtr as parent or call
model->setParent(tablePtr) prior to model.release()), mirroring presentToView()
behavior so models are deleted with their view.
- Around line 73-75: In QueryResultPresenter (around the view refresh code), the
existing code only calls view->setModel(nullptr) which detaches but does not
delete the previous model (the new models are parented to view later around the
create-model logic), causing accumulated QObject children; fix by explicitly
deleting the old model before detaching—e.g., if (auto old = view->model()) {
delete old; view->setModel(nullptr); }—so replace the current
view->setModel(nullptr) branch to delete view->model() first to prevent leaks.
---
Outside diff comments:
In `@src/database/dbexportdata.cpp`:
- Around line 40-43: The code returns early when QFile::open(...) fails without
recording the failure in ExportDataProgress, so upstream hasAnyErrors() may miss
it; update every early-return file-open failure (the file->open(...) checks
around the file write sections) to call ExportDataProgress to record the error
before returning — e.g. call progress.addError() or
ExportDataProgress::addError(...) with a descriptive message that includes the
file name (file->fileName()) and the QFile::errorString() (or errno/text) so the
failure is recorded for hasAnyErrors() and reporting.
---
Nitpick comments:
In `@src/database/dbanalyzer.cpp`:
- Around line 25-27: The analyze() flow currently lets loadTables() open and
close the DB then loadColumns() re-opens it, causing redundant churn; modify
analyze(), loadTables(), and loadColumns() so analyze() owns the open/close
lifecycle: in analyze() check database->isOpen(), call database->open() if
needed and remember whether you opened it, then call loadTables() and
loadColumns() without them performing open/close; remove or guard the
database->open()/close() calls inside loadTables() and loadColumns() (or make
them conditional on not already-open) and ensure at the end of analyze() you
only call database->close() if you were the opener (using the saved wasOpen
flag).
🪄 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: 4d2f6d55-a93c-43de-ae3c-60903ad252dd
📒 Files selected for processing (23)
AGENTS.mdCONTEXT.mdbuild.ps1src/cli/script.cppsrc/database/dbanalyzer.cppsrc/database/dbexportdata.cppsrc/database/idatabase.hsrc/database/inmemorydatabase.cppsrc/database/inmemorydatabase.hsrc/database/progress.hsrc/database/queryexecutor.cppsrc/database/queryresult.hsrc/database/sqlitedatabase.cppsrc/database/sqlitedatabase.hsrc/gui/exportorchestrator.cppsrc/gui/exportorchestrator.hsrc/gui/mainwindow.cppsrc/gui/mainwindow.hsrc/gui/queryexecutionpresenter.cppsrc/gui/queryexecutionpresenter.hsrc/gui/queryresultpresenter.cpptests/CMakeLists.txttests/test_dbexportschema.cpp
| Write-Host "`nRunning tests..." | ||
| $testExe = "./build/SQLiteQueryTests" | ||
| if (Test-Path $testExe) { | ||
| $originalPath = $env:PATH | ||
| if ($QtPath) { | ||
| $pathArray = ($env:PATH -split ':') | Where-Object { $_ -ne "$QtPath/bin" -and $_ -ne "$QtPath/lib" } | ||
| $pathArray = @("$QtPath/bin", "$QtPath/lib") + $pathArray | ||
| $env:PATH = $pathArray -join ':' | ||
| } | ||
| Write-Host "Executing: $testExe" | ||
| & $testExe | ||
| if ($QtPath) { | ||
| $env:PATH = $originalPath | ||
| } | ||
| if ($LASTEXITCODE -ne 0) { | ||
| Write-Error "Tests failed (exit code: $LASTEXITCODE)" | ||
| exit 1 | ||
| } | ||
| Write-Host "`n=== All tests passed ===" -ForegroundColor Green | ||
| } | ||
| else { | ||
| Write-Warning "Test executable not found at: $testExe" | ||
| } |
There was a problem hiding this comment.
Inconsistent handling of missing test executables across platforms.
On Windows (lines 97-98), a missing test executable triggers Write-Error and exit 1. However, on Linux (line 133), it only emits Write-Warning and continues. This violates the stated PR objective that "missing test executables are treated as errors."
🔧 Proposed fix to make Linux consistent with Windows
else {
- Write-Warning "Test executable not found at: $testExe"
+ Write-Error "Test executable not found at: $testExe"
+ exit 1
}🤖 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 `@build.ps1` around lines 112 - 134, The script treats a missing test
executable differently on Linux vs Windows; locate the block that checks
Test-Path for $testExe (uses Test-Path, Write-Warning, Write-Host "Executing:
$testExe", and the $QtPath PATH manipulation) and change the else branch so it
mirrors the Windows behavior: replace the Write-Warning with a Write-Error
including the missing path and then call exit 1 so missing test executables are
treated as errors consistently across platforms; preserve the surrounding PATH
restore logic tied to $QtPath and ensure $LASTEXITCODE handling remains
unchanged after running the executable.
| if ($IsMacOS) { | ||
|
|
||
| cmake -B build -DCMAKE_BUILD_TYPE=Release -DCMAKE_INSTALL_PREFIX=/tmp/sqlitequery | ||
| cmake --build build --config Release --parallel 32 | ||
|
|
||
| Write-Host "`nRunning tests..." | ||
| $testExe = "./build/SQLiteQueryTests" | ||
| if (Test-Path $testExe) { | ||
| $originalPath = $env:PATH | ||
| if ($QtPath) { | ||
| $pathArray = ($env:PATH -split ':') | Where-Object { $_ -ne "$QtPath/bin" -and $_ -ne "$QtPath/lib" } | ||
| $pathArray = @("$QtPath/bin", "$QtPath/lib") + $pathArray | ||
| $env:PATH = $pathArray -join ':' | ||
| } | ||
| Write-Host "Executing: $testExe" | ||
| & $testExe | ||
| if ($QtPath) { | ||
| $env:PATH = $originalPath | ||
| } | ||
| if ($LASTEXITCODE -ne 0) { | ||
| Write-Error "Tests failed (exit code: $LASTEXITCODE)" | ||
| exit 1 | ||
| } | ||
| Write-Host "`n=== All tests passed ===" -ForegroundColor Green | ||
| } | ||
| else { | ||
| Write-Warning "Test executable not found at: $testExe" | ||
| } |
There was a problem hiding this comment.
Inconsistent handling of missing test executables across platforms.
On Windows (lines 97-98), a missing test executable triggers Write-Error and exit 1. However, on macOS (line 181), it only emits Write-Warning and continues. This violates the stated PR objective that "missing test executables are treated as errors."
🔧 Proposed fix to make macOS consistent with Windows
else {
- Write-Warning "Test executable not found at: $testExe"
+ Write-Error "Test executable not found at: $testExe"
+ exit 1
}🤖 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 `@build.ps1` around lines 155 - 182, The macOS branch currently treats a
missing test executable with Write-Warning which is inconsistent with the
Windows behavior; update the macOS test-executable handling so that when
Test-Path $testExe is false it calls Write-Error with a descriptive message and
then exits with exit 1 (same as Windows), ensuring $testExe, Test-Path,
Write-Error and exit 1 are used to signal a failure rather than just warning.
| - **ExportOrchestrator** — runs a DataExporter in the background, marshals | ||
| progress to the GUI thread, surfaces cancel and completion via exportToSql() | ||
| and exportToCsv(). |
There was a problem hiding this comment.
ExportOrchestrator is not a seam and should not be listed in the Seams section.
Per the coding guidelines, seams must be abstract interfaces with at least two adapters. ExportOrchestrator is a concrete orchestrator class, not an abstraction. This description already exists correctly in the Modules section (lines 44-45).
The Seams section should only list abstract interfaces like IDatabase. Remove ExportOrchestrator from here to avoid confusion about the architecture.
📐 Proposed fix
## Seams
- **IDatabase** — abstract Database. Production adapter is `SqliteDatabase`;
test adapter is `InMemoryDatabase`. Anything that takes a Database takes
`IDatabase*`. `QSqlDatabase` does not leak across this seam.
-- **ExportOrchestrator** — runs a DataExporter in the background, marshals
- progress to the GUI thread, surfaces cancel and completion via exportToSql()
- and exportToCsv().📝 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.
| - **ExportOrchestrator** — runs a DataExporter in the background, marshals | |
| progress to the GUI thread, surfaces cancel and completion via exportToSql() | |
| and exportToCsv(). | |
| ## Seams | |
| - **IDatabase** — abstract Database. Production adapter is `SqliteDatabase`; | |
| test adapter is `InMemoryDatabase`. Anything that takes a Database takes | |
| `IDatabase*`. `QSqlDatabase` does not leak across this seam. |
🤖 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 `@CONTEXT.md` around lines 55 - 57, Remove ExportOrchestrator from the Seams
section: edit the Seams list to only include abstract interfaces (e.g.,
IDatabase) and delete the ExportOrchestrator bullet/description
(ExportOrchestrator is a concrete orchestrator already documented under
Modules). Ensure the Seams section contains only true seams (abstract interfaces
with multiple adapters) and leave the Modules section entry for
ExportOrchestrator unchanged.
Source: Coding guidelines
| QList<QueryResult> results; | ||
| for (const auto &raw: statements) { | ||
| const QString sql = raw.trimmed().replace('\n', "", Qt::CaseInsensitive); | ||
| const QString sql = raw.trimmed().replace('\n', ""); |
There was a problem hiding this comment.
Replace newline stripping with whitespace normalization.
At Line 15, replacing '\n' with "" can merge tokens and break valid multiline SQL coming from the editor (SELECT ...\nFROM ... becomes SELECT ...FROM ... without spacing).
Suggested fix
- const QString sql = raw.trimmed().replace('\n', "");
+ QString sql = raw.trimmed();
+ sql.replace('\r', ' ');
+ sql.replace('\n', ' ');📝 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.
| const QString sql = raw.trimmed().replace('\n', ""); | |
| QString sql = raw.trimmed(); | |
| sql.replace('\r', ' '); | |
| sql.replace('\n', ' '); |
🤖 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/database/queryexecutor.cpp` at line 15, The current assignment to QString
sql uses raw.trimmed().replace('\n', "") which removes newlines and can glue
tokens; change it to normalize whitespace instead: replace newlines and other
whitespace runs with a single space and trim the result so multiline SQL like
"SELECT ...\nFROM ..." becomes "SELECT ... FROM ...". Update the expression that
sets sql (the line creating QString sql from raw) to perform a regex-based
whitespace collapse (e.g., replace sequences of whitespace characters with a
single space) and then call trimmed() so you preserve token separation while
still removing excess whitespace.
| } | ||
|
|
||
| void ExportOrchestrator::onExportComplete() { | ||
| tearingDown_ = true; |
There was a problem hiding this comment.
Reset tearingDown_ when starting a new export.
Line 39 latches tearingDown_ = true, but runExport never clears it. Subsequent exports can short-circuit polling/completion checks.
Suggested fix
void ExportOrchestrator::runExport(IDatabase *db, DatabaseInfo info,
std::function<void(IDatabase *, const DatabaseInfo &, const CancellationToken *, ExportDataProgress *)> fn) {
+ tearingDown_ = false;
progress_ = std::make_unique<ExportDataProgress>();
tcs_ = std::make_unique<CancellationTokenSource>();
completed_ = false;Also applies to: 64-69
🤖 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/gui/exportorchestrator.cpp` at line 39, The field tearingDown_ is set to
true during teardown but never reset, causing subsequent calls to runExport to
short-circuit; update the export startup logic (e.g., at the top of runExport or
wherever a new export job is initialized) to explicitly set tearingDown_ = false
before proceeding, and ensure any early-return paths that begin a fresh export
also reset tearingDown_; check the teardown path where tearingDown_ is set true
(the existing assignment) remains but mirror it with the reset at export start
so polling/completion logic sees the correct state.
| if (script.contains("create", Qt::CaseInsensitive) || | ||
| script.contains("drop", Qt::CaseInsensitive) || | ||
| script.contains("insert", Qt::CaseInsensitive) || | ||
| script.contains("delete", Qt::CaseInsensitive)) { | ||
| ui->queryResultTab->setCurrentIndex(1); | ||
| analyzeDatabase(); | ||
| } |
There was a problem hiding this comment.
Handle additional schema-changing SQL keywords in refresh trigger.
ALTER/RENAME scripts won’t trigger analyzeDatabase(), so tree/schema views can remain stale after successful execution.
Suggested patch
- if (script.contains("create", Qt::CaseInsensitive) ||
+ if (script.contains("create", Qt::CaseInsensitive) ||
+ script.contains("alter", Qt::CaseInsensitive) ||
+ script.contains("rename", Qt::CaseInsensitive) ||
script.contains("drop", Qt::CaseInsensitive) ||
script.contains("insert", Qt::CaseInsensitive) ||
script.contains("delete", Qt::CaseInsensitive)) {📝 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.
| if (script.contains("create", Qt::CaseInsensitive) || | |
| script.contains("drop", Qt::CaseInsensitive) || | |
| script.contains("insert", Qt::CaseInsensitive) || | |
| script.contains("delete", Qt::CaseInsensitive)) { | |
| ui->queryResultTab->setCurrentIndex(1); | |
| analyzeDatabase(); | |
| } | |
| if (script.contains("create", Qt::CaseInsensitive) || | |
| script.contains("alter", Qt::CaseInsensitive) || | |
| script.contains("rename", Qt::CaseInsensitive) || | |
| script.contains("drop", Qt::CaseInsensitive) || | |
| script.contains("insert", Qt::CaseInsensitive) || | |
| script.contains("delete", Qt::CaseInsensitive)) { | |
| ui->queryResultTab->setCurrentIndex(1); | |
| analyzeDatabase(); | |
| } |
🤖 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/gui/mainwindow.cpp` around lines 358 - 364, The refresh trigger only
looks for "create", "drop", "insert", "delete" in the SQL string, so
ALTER/RENAME statements don't call analyzeDatabase(); update the conditional
that examines the variable script (the same block that calls
ui->queryResultTab->setCurrentIndex(1) and analyzeDatabase()) to also detect
"alter" and "rename" (case-insensitive) so schema-changing statements trigger ui
refresh; keep the same behavior of switching to the results tab before calling
analyzeDatabase().
| exportOrchestrator->exportToSql(std::move(info), filepath, database.get()); | ||
|
|
||
| connect(exportOrchestrator.get(), &ExportOrchestrator::exportProgress, | ||
| this, &MainWindow::onExportProgress); | ||
| this, &MainWindow::onExportProgress, Qt::UniqueConnection); | ||
| connect(exportOrchestrator.get(), &ExportOrchestrator::exportCompleted, | ||
| this, &MainWindow::onExportCompleted); | ||
| this, &MainWindow::onExportCompleted, Qt::UniqueConnection); |
There was a problem hiding this comment.
Connect export signals before starting export work.
connect(...) currently runs after exportToSql/exportToCsv(...). If export emits completion/progress immediately (fast path or early failure), slots can miss signals and leave actions disabled or status stale.
Suggested patch
- exportOrchestrator->exportToSql(std::move(info), filepath, database.get());
-
connect(exportOrchestrator.get(), &ExportOrchestrator::exportProgress,
this, &MainWindow::onExportProgress, Qt::UniqueConnection);
connect(exportOrchestrator.get(), &ExportOrchestrator::exportCompleted,
this, &MainWindow::onExportCompleted, Qt::UniqueConnection);
+
+ exportOrchestrator->exportToSql(std::move(info), filepath, database.get());
@@
- exportOrchestrator->exportToCsv(std::move(info), outputFolder, delimiter, database.get());
-
connect(exportOrchestrator.get(), &ExportOrchestrator::exportProgress,
this, &MainWindow::onExportProgress, Qt::UniqueConnection);
connect(exportOrchestrator.get(), &ExportOrchestrator::exportCompleted,
this, &MainWindow::onExportCompleted, Qt::UniqueConnection);
+
+ exportOrchestrator->exportToCsv(std::move(info), outputFolder, delimiter, database.get());Also applies to: 418-423
🤖 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/gui/mainwindow.cpp` around lines 396 - 401, Move the connect calls to
attach ExportOrchestrator signals before invoking export work so slots can't
miss immediate progress/completion; specifically, for calls like
exportOrchestrator->exportToSql(std::move(info), filepath, database.get()) and
exportOrchestrator->exportToCsv(...), call connect(exportOrchestrator.get(),
&ExportOrchestrator::exportProgress, this, &MainWindow::onExportProgress,
Qt::UniqueConnection) and connect(exportOrchestrator.get(),
&ExportOrchestrator::exportCompleted, this, &MainWindow::onExportCompleted,
Qt::UniqueConnection) first, then start the export; ensure you preserve the
std::move(info) semantics and apply the same reorder for the other block
(exportToCsv).
| auto model = std::make_unique<QStandardItemModel>( | ||
| static_cast<int>(result.rows.size()), | ||
| static_cast<int>(result.columns.size()), | ||
| container.get()); | ||
| nullptr); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/gui/queryresultpresenter.cpp"
# Ensure file exists and show relevant surrounding code
ls -la "$FILE"
# Show the region around the reported lines
sed -n '1,140p' "$FILE" | nl -ba | sed -n '1,140p'
# Specifically extract around the snippet and suggested fix context (lines ~35-110 in file)
sed -n '35,110p' "$FILE" | nl -ba
# Search for present(), presentToView(), setModel(), setParent(), release()/get()
rg -n "presentToView|present\s*\(|setModel\(|setParent\(|release\(|unique_ptr<|QStandardItemModel" "$FILE"Repository: christianhelle/sqlitequery
Length of output: 214
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/gui/queryresultpresenter.cpp"
echo "== Exists =="
test -f "$FILE" && echo "found $FILE"
echo "== First 160 lines (with line numbers via awk) =="
awk '{printf "%5d %s\n", NR, $0}' "$FILE" | sed -n '1,160p'
echo "== Focus around reported area (lines 25-120) =="
awk '{printf "%5d %s\n", NR, $0}' "$FILE" | sed -n '25,120p'
echo "== Search for model ownership / setModel / setParent / release usage =="
rg -n "presentToView|present\s*\(|setModel\(|setParent\(|release\(|unique_ptr<|QStandardItemModel" "$FILE"Repository: christianhelle/sqlitequery
Length of output: 7689
present() leaks QStandardItemModel instances (models have no parent and aren’t deleted when QTableViews are destroyed).
In QueryResultPresenter::present(), the model is created with a nullptr QObject parent (lines 43-46) and then ownership is released into tablePtr->setModel(model.release()) (lines 59-61). When clear()/present() later qDeleteAll(this->tableViews...), the QTableView deletion does not free the unparented QStandardItemModel, so repeated calls accumulate leaked models.
presentToView() is different: it sets model->setParent(view) (lines 93-94), so that path doesn’t have the same leak.
Suggested fix
- auto model = std::make_unique<QStandardItemModel>(
+ auto model = std::make_unique<QStandardItemModel>(
static_cast<int>(result.rows.size()),
static_cast<int>(result.columns.size()),
nullptr);
@@
const auto tablePtr = new QTableView(container.get());
+ model->setParent(tablePtr);
tablePtr->setModel(model.release());🤖 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/gui/queryresultpresenter.cpp` around lines 43 - 47,
QueryResultPresenter::present() currently creates a QStandardItemModel with a
nullptr parent and then releases it to tablePtr->setModel(...), causing leaked
models; fix by giving the model a QObject parent tied to the QTableView before
releasing ownership (either construct the QStandardItemModel with tablePtr as
parent or call model->setParent(tablePtr) prior to model.release()), mirroring
presentToView() behavior so models are deleted with their view.
| if (view->model() != nullptr) { | ||
| view->setModel(nullptr); | ||
| } |
There was a problem hiding this comment.
src/gui/queryresultpresenter.cpp (Lines 73-75): delete old model when refreshing the same view.
view->setModel(nullptr) only detaches; since the new model is parented to view (around Lines 93-94), repeated refreshes keep prior models alive as QObject children until view is destroyed.
Suggested fix
- if (view->model() != nullptr) {
- view->setModel(nullptr);
- }
+ if (auto *oldModel = view->model(); oldModel != nullptr) {
+ view->setModel(nullptr);
+ if (oldModel->parent() == view) {
+ oldModel->deleteLater();
+ }
+ }🤖 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/gui/queryresultpresenter.cpp` around lines 73 - 75, In
QueryResultPresenter (around the view refresh code), the existing code only
calls view->setModel(nullptr) which detaches but does not delete the previous
model (the new models are parented to view later around the create-model logic),
causing accumulated QObject children; fix by explicitly deleting the old model
before detaching—e.g., if (auto old = view->model()) { delete old;
view->setModel(nullptr); }—so replace the current view->setModel(nullptr) branch
to delete view->model() first to prevent leaks.
1 participant
This pull request introduces several improvements and bug fixes across the build scripts, database modules, error handling, and export orchestration logic. The main changes enhance cross-platform test execution, improve error reporting during exports and script execution, and ensure safer resource management in asynchronous GUI operations. Several interface and implementation details have also been clarified or refactored for maintainability.
Build and Test Improvements
build.ps1for Windows, Linux, and macOS, ensuring that tests are always run after building and that errors are handled consistently. Now, missing test executables are treated as errors, and environment variables for Qt are managed more robustly. [1] [2] [3]Database and Export Error Handling
ExportDataProgressnow tracks errors per table, and errors are surfaced to the user when they occur during export. [1] [2] [3] [4]IDatabaseinterface now includes anisOpen()method, and bothSqliteDatabaseandInMemoryDatabaseimplement it, allowing for safer connection management. [1] [2] [3]Resource and Concurrency Safety in GUI Export
ExportOrchestratornow usesQPointerand atearingDown_flag to prevent race conditions and use-after-free bugs during asynchronous export operations and progress polling. [1] [2] [3] [4]Refactoring and Maintenance
SqliteDatabaseby assigning unique connection names and only opening the database if not already open. [1] [2]DbAnalyzerto avoid unnecessary open/close cycles and only close the database if it was not already open. [1] [2]Documentation and Context Updates
ExportStrategywithExportOrchestratorand clarifying the definition of a Query. [1] [2] [3]These changes collectively improve reliability, maintainability, and user feedback in both the CLI and GUI applications.
Summary by CodeRabbit
New Features
Bug Fixes
Chores