Repository navigation
Deepen SQLite Query Analyzer architecture - #110
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR refactors the SQLite Query Analyzer from a concrete ChangesDatabase Architecture and Query Execution Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
🚥 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
src/SQLIteAnalyzer.pro (1)
18-40:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winBuild system inconsistency: qmake source list contains files removed from CMake.
The qmake
SOURCESlist still includesdatabase/dbquery.cpp(line 25) andgui/exportstrategy.cpp(line 39), but these files were removed fromCMakeLists.txtaccording to the PR objectives (DbQuery was refactored into QueryExecutor, ExportStrategy was collapsed into ExportOrchestrator). Similarly, the corresponding headers remain at lines 51 and 67.This will cause qmake builds to fail when the removed source files are not found, or produce incorrect builds if stale files remain.
🔧 Proposed fix to sync qmake with CMake
Remove the obsolete entries from SOURCES:
gui/mainwindow.cpp \ database/dbanalyzer.cpp \ database/queryexecutor.cpp \ gui/highlighter.cpp \ gui/queryresultpresenter.cpp \ - database/dbquery.cpp \ database/sqlitedatabase.cpp \gui/prompts.cpp \ cli/export.cpp \ cli/script.cpp \ gui/exportorchestrator.cpp \ - gui/exportstrategy.cpp \ gui/sessionmanager.cppRemove the obsolete entries from HEADERS:
gui/highlighter.h \ gui/queryresultpresenter.h \ - database/dbquery.h \ database/sqlitedatabase.h \cli/export.h \ cli/script.h \ gui/exportorchestrator.h \ - gui/exportstrategy.h \ gui/sessionmanager.h🤖 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/SQLIteAnalyzer.pro` around lines 18 - 40, The qmake project file still lists removed source/header files causing build mismatch: remove the obsolete entries database/dbquery.cpp and gui/exportstrategy.cpp from the SOURCES block in SQLIteAnalyzer.pro and also remove their corresponding header entries (e.g., dbquery.h and exportstrategy.h) from the HEADERS section so the .pro matches the refactored code (QueryExecutor/QueryExecutor.cpp and ExportOrchestrator/*.cpp now used); ensure no other references to these filenames remain in the .pro.src/cli/script.cpp (1)
29-35:⚠️ Potential issue | 🟠 Major | ⚡ Quick winExecution errors are gathered but never surfaced to CLI output.
Line 29–Line 31 collects statement errors, but Line 33–Line 35 always prints elapsed time only. Failed statements become silent failures for users and automation.
Suggested fix
QueryExecutor executor(database.get()); QStringList errors; executor.runScript(sqlScript, &errors); + if (!errors.isEmpty()) { + QTextStream err(stderr); + for (const auto &e : errors) { + err << e << "\n"; + } + } const auto milliseconds = static_cast<double>(time.elapsed());🤖 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/cli/script.cpp` around lines 29 - 35, The collected execution errors in the QStringList errors from executor.runScript(sqlScript, &errors) are never shown; modify the CLI to check the errors list after runScript and emit each error to the console (e.g., via QTextStream to stderr or stdout) before or after printing the elapsed time (msg), and ensure the process exits with a non-zero code when errors is non-empty; update the code around executor.runScript / errors / msg to iterate errors and output them so failed statements are visible to users and automation.src/gui/exportorchestrator.cpp (1)
38-56:⚠️ Potential issue | 🔴 CriticalFix race/crash in onExportComplete vs progress polling (progress_.release nulls progress_)
onExportComplete()callsprogress_.release()/tcs_.release()(leaks + sets both members tonullptr), while the polling loop still dereferencesprogress_inemit exportProgress(progress_->getAffectedRows())and in the loop condition!progress_->isCompleted(). There’s a window beforecompleted_ = truewhere this can crash or emit from invalid state. Use a single synchronized ownership/stop mechanism (don’trelease(); keepprogress_/tcs_alive until polling has exited, or capture the progress pointer/shared ownership used by the poller and only tear down after both export + polling are done).🤖 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` around lines 38 - 56, The race is caused by onExportComplete calling progress_.release()/tcs_.release() while startProgressPolling's background lambda still dereferences progress_; to fix, stop releasing raw ownership and instead use synchronized/shared ownership and a stop signal: keep progress_ and tcs_ alive (don't call release()), set completed_ (or request cancellation on the provided CancellationToken) before tearing down, or capture a shared/shared_ptr copy of progress_ inside the QtConcurrent lambda so the poller holds ownership; ensure onExportComplete signals cancellation and waits for the poller to exit (or relies on the token) before resetting progress_/tcs_; update ExportOrchestrator::onExportComplete and startProgressPolling to use these coordinated steps (avoid direct release() while poller runs).src/gui/mainwindow.cpp (2)
342-348:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMove SQL script splitting out of
MainWindow.Splitting on
";"in GUI code is functionally unsafe (e.g., semicolons inside literals/trigger bodies) and keeps parsing logic outside the database module. LetQueryExecutor/database code accept the raw script and split there.Based on learnings: "No SQL string concatenation in non-database modules. Query splitting logic must live in the
databasemodule, not inmainwindow.cppor the CLI."🤖 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 342 - 348, MainWindow is splitting the SQL script with QString::split(";") and passing QStringList to queryPresenter->execute which moves parsing logic into the GUI; instead change MainWindow to pass the raw script (e.g., ui->textEdit->toPlainText()) to queryPresenter->execute (update its signature to accept const QString& script or QByteArray and error output), and implement the semicolon-aware SQL splitting inside the database module / QueryExecutor (or QueryPresenter backend) so it can correctly handle semicolons in literals/triggers; update all callers of queryPresenter->execute to the new signature and ensure errors are still populated via the existing errors parameter.Source: Learnings
400-403:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid reconnecting export signals on every export run.
These
connect(...)calls execute each time export starts, so handlers stack up and fire multiple times after repeated exports. Connect once (e.g., in constructor) or useQt::UniqueConnection.💡 Suggested change
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);Also applies to: 422-425
🤖 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 400 - 403, The connect calls that wire ExportOrchestrator::exportProgress and ExportOrchestrator::exportCompleted to MainWindow::onExportProgress and MainWindow::onExportCompleted are being run on every export and causing handlers to stack; either move these connect(...) calls so they execute once (e.g., into MainWindow constructor where exportOrchestrator is created/assigned) or add the Qt::UniqueConnection flag to the connect calls to prevent duplicate connections; update both the exportProgress/exportCompleted connects (including the ones around lines 422-425) accordingly.src/database/dbexportdata.cpp (2)
35-73:⚠️ Potential issue | 🟠 Major | ⚡ Quick winExport silently ignores streaming errors.
When
streamRowsfails (line 66-68), the code continues to the next table without reporting the error to the caller. Users won't know if the export is incomplete or if specific tables failed to export.Consider accumulating errors in
ExportDataProgressor returning a status list so callers can detect and report partial failures.🛡️ Log or accumulate errors
const QueryResult streamResult = database->streamRows( QString("SELECT * FROM \"%1\"").arg(table.name), [&](const QList<QVariant> &values) { if (cancellationToken->isCancellationRequested()) return false; const auto valueList = getColumnValueDefs(table, values).join(", "); out << "INSERT INTO \"" << table.name << "\"(" << columns << ") "; out << "VALUES (" << valueList << ");\n"; progress->increment(); return true; }); if (!streamResult.ok) { + qWarning() << "Failed to export table" << table.name << ":" << streamResult.error; continue; }Better: extend
ExportDataProgresswith an error list and populate it, so GUI/CLI can surface failures.🤖 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 35 - 73, DbDataExport::exportDataToSqlFile currently swallows streamRows failures (QueryResult.ok false) and continues silently; change the failure path to record the error into ExportDataProgress (e.g., call a new method like progress->addError(table.name, streamResult.errorMessage) or progress->reportTableFailure(table.name, streamResult.errorMessage)) so callers can detect partial exports, and ensure setCompleted semantics reflect presence of errors (e.g., only mark completed when no errors or provide a final progress->setFinishedWithErrors/count). Update ExportDataProgress to expose the chosen error-accumulation API and use that API in the !streamResult.ok branch of exportDataToSqlFile instead of silently continuing.
75-114:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCSV export silently ignores streaming errors.
Similar to SQL export above, when
streamRowsfails (lines 108-110), the code continues without reporting the error. This masks partial export failures from the user.🛡️ Log or accumulate errors
const QueryResult streamResult = database->streamRows( QString("SELECT * FROM \"%1\"").arg(table.name), [&](const QList<QVariant> &values) { if (cancellationToken->isCancellationRequested()) return false; const auto valueList = getColumnValueDefs(table, values).join(delimiter); out << valueList << "\n"; progress->increment(); return true; }); file->close(); if (!streamResult.ok) { + qWarning() << "Failed to export table" << table.name << ":" << streamResult.error; continue; }🤖 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 75 - 114, The CSV export currently ignores streamRows failures; after calling database->streamRows (in DbDataExport::exportDataToCsvFile) check streamResult.ok and when false report the error instead of silently continuing: call the appropriate error reporting on ExportDataProgress (e.g., progress->setError or progress->addError) with a message including streamResult.errorMessage, table.name and filename, close and/or remove the partial file, and return early (or otherwise mark overall export failed) so callers see the failure; ensure you include the streamResult.errorMessage from QueryResult in the report.
🧹 Nitpick comments (7)
build.ps1 (1)
84-87: ⚡ Quick winPATH manipulation is fragile and may leave duplicates.
Line 84 prepends Qt directories to
PATHwithout checking if they're already present, potentially creating duplicates. Line 87's cleanup uses regex replacement assuming the Qt paths are at a specific position with exact format"$QtPath\bin;$QtPath\lib;". If the PATH was modified elsewhere or doesn't match this exact format, the cleanup will fail silently, leaving the session PATH polluted.♻️ More robust PATH handling
- $env:PATH = "$QtPath\bin;$QtPath\lib;$env:PATH" + $originalPath = $env:PATH Write-Host "Executing: $testExe" + $env:PATH = "$QtPath\bin;$QtPath\lib;$originalPath" & $testExe - $env:PATH = $env:PATH -replace [regex]::Escape("$QtPath\bin;$QtPath\lib;"), "" + $env:PATH = $originalPath if ($LASTEXITCODE -ne 0) {🤖 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 84 - 87, The script currently mutates $env:PATH by naively prepending "$QtPath\bin;$QtPath\lib;" which can create duplicates and the regex-based cleanup on $env:PATH can fail; change the logic in the build step that touches $env:PATH (references: $env:PATH, $QtPath, $testExe) to instead save $originalPath = $env:PATH before modification, build a PATH array from $env:PATH -split ';', insert "$QtPath\bin" and "$QtPath\lib" only if they are not already present, set $env:PATH to the joined array, run & $testExe, and finally restore $env:PATH = $originalPath (or rebuild the PATH array to remove only the two Qt entries reliably) to avoid leaving duplicates or polluted session state.tests/CMakeLists.txt (1)
46-52: Clarify whyQt6::Guiis linked for the DB/export test target
Qt6::Guilooks necessary: the test target links${PROJECT_NAME}withQt6::Gui, and it buildssrc/gui/exportorchestrator.cpp;src/gui/exportorchestrator.hincludessrc/threading/mainthread.h, which includes<QGuiApplication>and usesqAppforQt::QueuedConnection. DroppingQt6::Guiwould require makingMainThreadQtCore-only (e.g., switch toQCoreApplication/remove theQGuiApplicationdependency).🤖 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 `@tests/CMakeLists.txt` around lines 46 - 52, The test target currently links Qt6::Gui because src/gui/exportorchestrator.cpp (and its header exportorchestrator.h) pulls in src/threading/mainthread.h which depends on QGuiApplication and uses qApp with Qt::QueuedConnection; if you want to remove Qt6::Gui from target_link_libraries then make MainThread QtCore-only (e.g., remove or replace QGuiApplication usage and qApp dependency and avoid Qt::QueuedConnection usage requiring a QGuiApplication), or alternatively keep Qt6::Gui and add a clarifying comment in CMakeLists.txt explaining that Qt6::Gui is required due to MainThread’s reliance on QGuiApplication/qApp used by exportorchestrator.cpp.tests/test_dbexportschema.cpp (1)
71-79: ⚡ Quick winDocumented limitation: FOREIGN KEY and UNIQUE constraints are lost during analysis.
The comment correctly documents that FK and UNIQUE constraints are not captured by the
Columnstruct duringDbAnalyzer.analyze(), meaning exported schemas will be incomplete. This is a known architectural limitation per the PR objectives, but it may surprise users who expect full schema fidelity.Consider adding a note to user-facing documentation that schema export currently supports only PRIMARY KEY and NOT NULL constraints, or extend the
Columnstruct to capture FK/UNIQUE in a future iteration.🤖 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 `@tests/test_dbexportschema.cpp` around lines 71 - 79, The test comment reveals a documented limitation: FOREIGN KEY and UNIQUE aren’t captured by Column during DbAnalyzer.analyze(), so exported schemas omit them; update user-facing documentation (and optionally add an inline TODO) to state that schema export currently preserves only PRIMARY KEY and NOT NULL constraints — mention DbSchemaExport/exportSchema, DbAnalyzer.analyze, and the Column struct by name so users/readers know where the limitation originates, and leave a short note to consider extending Column to store FK/UNIQUE in a future iteration.src/database/inmemorydatabase.cpp (1)
10-10: 💤 Low valueRemove unnecessary setHostName for SQLite.
SQLite is a file-based embedded database;
setHostName("localhost")has no effect. Remove this call for clarity.🤖 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/inmemorydatabase.cpp` at line 10, Remove the unnecessary call to database.setHostName("localhost") in src/database/inmemorydatabase.cpp: since SQLite is file-based, delete the setHostName invocation (referencing the database object and its setHostName method) so the code only configures relevant SQLite parameters and avoids misleading/no-op host configuration.src/database/dbanalyzer.cpp (1)
26-51: ⚡ Quick winConsider checking if database is already open before calling open().
Line 29 unconditionally calls
database->open(), which may close an already-open connection and reopen it (depending on the adapter's implementation). If the caller has already opened the database, this can be wasteful or disruptive.Check
isOpen()first, or document thatloadTablesexpects a closed database.♻️ Check before opening
void DbAnalyzer::loadTables(DatabaseInfo &info) const { const QString sql = "SELECT * FROM sqlite_master WHERE type='table'"; - if (!this->database->open()) { - return; + const bool wasOpen = database->isOpen(); + if (!wasOpen && !database->open()) { + return; } const QueryResult result = this->database->runStatement(sql); if (!result.ok) { - database->close(); + if (!wasOpen) + database->close(); return; } const int nameIdx = result.columns.indexOf("name"); for (const auto &row: result.rows) { const QString name = row.values.at(nameIdx).toString(); if (name == "sqlite_sequence" || name == "sqlite_stat1") { continue; } Table table; table.name = name; info.tables.append(table); } - database->close(); + if (!wasOpen) + database->close(); }Alternatively, require the caller to open the database and remove open/close from this method entirely.
🤖 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 26 - 51, DbAnalyzer::loadTables currently calls database->open() unconditionally which can reopen an already-open connection; modify loadTables to check database->isOpen() first and only call database->open() when not already open, and ensure you only call database->close() if this method opened the connection (track a local flag like openedHere) so you don't close a caller-owned connection; update references inside loadTables (the open/close logic around this->database, QueryResult handling, and final database->close()) accordingly or alternatively remove open/close from loadTables and document that the caller must open the Database before calling loadTables on DatabaseInfo.src/database/sqlitedatabase.cpp (2)
9-9: 💤 Low valueRemove unnecessary setHostName for SQLite.
SQLite is a file-based embedded database;
setHostName("localhost")has no effect and is misleading. This call should be removed.♻️ Proposed cleanup
SqliteDatabase::SqliteDatabase() { database = QSqlDatabase::addDatabase("QSQLITE"); - database.setHostName("localhost"); }🤖 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/sqlitedatabase.cpp` at line 9, Remove the unnecessary call to database.setHostName("localhost") in the SQLite connection setup: SQLite is file-based and does not use host settings, so delete the setHostName(...) invocation (the one called on the database variable in the SqliteDatabase/SQLite connection initialization) to avoid misleading code and comments.
18-21: 💤 Low valueRedundant close before open.
Calling
close()beforeopen()is unnecessary when the database is not already open. This adds overhead and makes the open flow less clear.♻️ Streamline the open logic
bool SqliteDatabase::open() { - this->close(); + if (database.isOpen()) + database.close(); return database.open(); }🤖 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/sqlitedatabase.cpp` around lines 18 - 21, The SqliteDatabase::open implementation unnecessarily calls this->close() before opening; remove that redundant call and implement open to either return true if database.isOpen() or otherwise call database.open() (use the existing database.isOpen() and database.open() methods), so SqliteDatabase::open no longer always closes before attempting to open.
🤖 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 `@AGENTS.md`:
- Around line 36-38: Update the stale reference in AGENTS.md to name the current
symbol/module: change the SQL-splitting rule text that mentions "dbquery.h" to
reference the database module's QueryExecutor (or the appropriate class/function
responsible for SQL splitting), and clarify that splitting-by-`;` is implemented
in the database module (QueryExecutor) not in mainwindow.cpp or the CLI so
future edits aren't anchored to removed files.
In `@build.ps1`:
- Around line 94-96: The else branch that currently calls Write-Warning when the
test executable referenced by $testExe is missing should instead fail the build:
replace the warning with a failing action (e.g., Write-Error with the same
message and then exit 1 or throw) inside the else block so the script terminates
with a non-zero exit code when $testExe is not found; update the branch that
checks $testExe in build.ps1 to stop execution rather than continue.
- Around line 80-97: Add the same test-execution logic you have in the Windows
block to the Linux and macOS sections immediately after their build steps: set
$testExe to the built test binary (use "./build/SQLiteQueryTests" without .exe
on Unix), optionally prepend "$QtPath/bin;$QtPath/lib" to $env:PATH if $QtPath
is set, log and execute & $testExe, check $LASTEXITCODE and call exit 1 with
Write-Error on failure, restore $env:PATH (remove the injected Qt path) and emit
a success message on pass, and emit a warning if the test binary is not found;
reference the same symbols ($testExe, $env:PATH, $QtPath, $LASTEXITCODE) so the
logic mirrors the Windows block exactly.
In `@CONTEXT.md`:
- Line 25: Update the wording of the "Query" definition to use clearer phrasing:
replace "a single SQL statement the user wrote and wants executed." with either
"a single SQL statement the user wrote and wants to execute." or "a single SQL
statement the user wrote and wants to be executed." so the definition for
"Query" reads cleanly and unambiguously.
- Around line 55-56: Remove the stale "ExportStrategy" entry from CONTEXT.md and
update the vocabulary to reference the new orchestrator-based API: replace
mentions of ExportStrategy (and any references to
SqlExportStrategy/CsvExportStrategy) with ExportOrchestrator and its methods
exportToSql() and exportToCsv(), ensuring the doc reflects the current design
where ExportOrchestrator provides export entrypoints instead of a strategy seam.
In `@src/database/dbanalyzer.cpp`:
- Around line 5-8: Remove the duplicate inclusion of "dbanalyzer.h" in
dbanalyzer.cpp so the file only includes it once; locate both occurrences of
`#include` "dbanalyzer.h" and delete the redundant one, leaving the single include
alongside `#include` <QFileInfo>, and verify the dbanalyzer.h header has proper
include guards or `#pragma` once to prevent future duplicate-include issues.
In `@src/database/inmemorydatabase.cpp`:
- Around line 37-70: The runStatement and streamRows logic in InMemoryDatabase
(methods InMemoryDatabase::runStatement and its streamRows counterpart)
duplicates the identical implementations in SqliteDatabase; extract the shared
behavior into a single helper (e.g., a QtSqlExecutor with static methods
runStatement(QSqlDatabase&, const QString&) and streamRows(QSqlDatabase&, const
QString&, std::function<bool(const QList<QVariant>&)>)) and have both
SqliteDatabase::runStatement/streamRows and
InMemoryDatabase::runStatement/streamRows delegate to those helper methods;
ensure the helper reproduces existing behavior (open check, error capturing via
QSqlError::text(), building columns/rows, isSelect handling and rowsAffected) so
fixes (like VACUUM error checks) apply in one place.
- Around line 13-16: The current InMemoryDatabase::setSource incorrectly calls
database.setDatabaseName(filename) on an in-memory DB; update setSource to
detect an in-memory database (check database.databaseName() == ":memory:" or
similar) and do not attempt to change its name — instead set the source member
but either log a clear warning and return early or throw an exception to reject
the operation; ensure you reference InMemoryDatabase::setSource, the source
member, database.databaseName(), and avoid calling database.setDatabaseName()
when the DB is in-memory.
In `@src/database/inmemorydatabase.h`:
- Line 28: Remove the concrete Qt DB leak by deleting the getConnection() method
(the [[nodiscard]] QSqlDatabase getConnection() const accessor) from
InMemoryDatabase so the class only exposes the IDatabase interface; then update
any callers to stop using getConnection() and instead call the public IDatabase
methods or, for tests that truly need raw access, add a test-only subclass or
friend accessor inside the test code (not in production headers) that exposes
the QSqlDatabase for testing only.
In `@src/database/queryexecutor.cpp`:
- Around line 12-27: The replace call in QueryExecutor::runStatements uses
Qt::CaseInsensitive unnecessarily when replacing '\n' (no case); update the sql
construction so the newline replacement omits the Qt::CaseSensitivity argument
(i.e., call QString::replace with the QChar and replacement only) to remove the
redundant parameter and improve clarity while keeping the trim/replace logic and
subsequent error handling in QueryExecutor::runStatements unchanged.
In `@src/database/queryresult.h`:
- Line 19: The field rowsAffected in QueryResult is currently declared as int
which can overflow for large DML counts; change its type to qint64 to match Qt
SQL row-count semantics and avoid truncation—update the declaration of
rowsAffected in the QueryResult class/struct in queryresult.h from int to
qint64, and ensure any code that constructs or reads this member (e.g.,
constructors, accessors or assignments referring to rowsAffected) is updated to
use qint64 as well.
In `@src/database/sqlitedatabase.cpp`:
- Around line 28-34: In SqliteDatabase::shrink, check the boolean result of
query.exec("VACUUM") and handle failures by logging the error details (use
QSqlQuery::lastError().text() and optionally QSqlDatabase::lastError().text())
instead of ignoring the return value; update the code that creates QSqlQuery
query(database) and call query.exec(...) inside an if/else (or capture the
result) and emit a warning or error (e.g., via qWarning()/logger) with a clear
message that VACUUM failed and include the error text to aid debugging.
- Around line 7-10: The SqliteDatabase constructor currently calls
QSqlDatabase::addDatabase("QSQLITE") which creates the default connection and
causes collision when multiple SqliteDatabase instances exist; change the
constructor to call addDatabase with a unique connection name (e.g., generate
one with a static counter or QUuid) so each SqliteDatabase instance has its own
connection; update any places that assume the default connection to use
database.connectionName() if needed and ensure the generated name is stored on
the instance for later reference (refer to SqliteDatabase::SqliteDatabase and
the use of QSqlDatabase::addDatabase).
In `@src/gui/exportorchestrator.cpp`:
- Around line 72-75: The continuation passed to future.then captures raw this
and posts another raw-this lambda to MainThread::run, causing a UAF if
ExportOrchestrator is destroyed before those runnables execute; change the
continuation to capture a lifetime-guard (e.g., QPointer<ExportOrchestrator> or
std::weak_ptr from enabling shared_from_this) and have the posted MainThread
lambda check that guard before calling ExportOrchestrator::onExportComplete (and
access progress_/tcs_ only after validating the guard). Also update the class
teardown (cancel()) to mark/clear a sentinel (or reset the QPointer/shared_ptr)
so any already-queued main-thread lambdas see the guard as invalid and return
early; apply the same pattern to startProgressPolling callbacks that post [this]
lambdas.
In `@src/gui/mainwindow.h`:
- Line 13: MainWindow currently includes and owns the concrete SqliteDatabase;
change it to depend on the abstract database interface instead by removing the
SqliteDatabase include and holding a pointer/reference to the interface (e.g.,
Database or IDatabase) in the MainWindow class; update MainWindow's
constructor/signature to accept an interface pointer/reference (or
std::unique_ptr<Database>) so the concrete SqliteDatabase is created/injected
elsewhere (factory/DI) and replace any direct SqliteDatabase usage in methods
with the interface methods (locate symbols: MainWindow class, its constructor,
and the member currently typed as SqliteDatabase).
In `@src/gui/queryresultpresenter.cpp`:
- Around line 25-33: QueryResultPresenter::present currently appends new table
widgets on each call without clearing prior state; before iterating results,
remove and delete any existing widgets stored in tableViews from this->widget
(or its layout), clear the tableViews container, and reset any related
offsets/counters (yOffset, count) so the new render starts fresh; ensure you
both remove the widgets from the parent (or layout) and delete them to avoid
leaks, then proceed with the existing loop to create and store new table views.
- Around line 69-87: presentToView currently creates a new QStandardItemModel
and releases it into view->setModel() while leaving previous models alive; fix
by (1) constructing the QStandardItemModel without parenting it to the view (use
nullptr parent when creating the model in QueryResultPresenter::presentToView)
and (2) before calling view->setModel(model.release()), check view->model() and
if non-null call view->setModel(nullptr) then delete the old model pointer to
free the previous QStandardItemModel; this ensures no accumulation of orphaned
models when presentToView is called repeatedly.
- Around line 40-57: present() creates a QStandardItemModel with container.get()
as parent then does model.release() and setModel(...), so when clear() deletes
only QTableView objects the models (children of container) leak across runs; fix
by making the model a child of the QTableView so it is deleted with the view:
create the QTableView (tablePtr) first, then construct the QStandardItemModel
with tablePtr as its parent (or call model->setParent(tablePtr) before
model.release()), and then call tablePtr->setModel(model.release()); also ensure
clear() continues to qDeleteAll(this->tableViews...) so views and their child
models are destroyed together (referencing present(), clear(), the
QStandardItemModel construction and tablePtr->setModel calls).
In `@tests/CMakeLists.txt`:
- Line 7: Replace the hardcoded Qt6 find_package in tests' CMakeLists.txt with
the same flexible pattern used in the main CMakeLists: use find_package(QT NAMES
Qt5 Qt6 REQUIRED COMPONENTS Core Sql) to allow either Qt5 or Qt6, and update any
target_link_libraries references (e.g., the test target that currently links
Qt::Core/Qt::Sql or Qt6::Core/Qt6::Sql) to use the imported targets produced by
that find_package (such as Qt${QT_VERSION_MAJOR}::Core and
Qt${QT_VERSION_MAJOR}::Sql or the generic QT targets from find_package) so
linking works for both Qt5 and Qt6.
- Around line 31-42: Remove the header-only progress.h from the PROD_SRCS list
so it is not treated as a compilable source: edit the CMake variable PROD_SRCS
and delete the entry "../src/database/progress.h" (leave other entries intact)
so that only .cpp files are compiled; if progress.h needs to be available to
targets, ensure the directory is reachable via target_include_directories or
target_include_directories for the target that uses it.
---
Outside diff comments:
In `@src/cli/script.cpp`:
- Around line 29-35: The collected execution errors in the QStringList errors
from executor.runScript(sqlScript, &errors) are never shown; modify the CLI to
check the errors list after runScript and emit each error to the console (e.g.,
via QTextStream to stderr or stdout) before or after printing the elapsed time
(msg), and ensure the process exits with a non-zero code when errors is
non-empty; update the code around executor.runScript / errors / msg to iterate
errors and output them so failed statements are visible to users and automation.
In `@src/database/dbexportdata.cpp`:
- Around line 35-73: DbDataExport::exportDataToSqlFile currently swallows
streamRows failures (QueryResult.ok false) and continues silently; change the
failure path to record the error into ExportDataProgress (e.g., call a new
method like progress->addError(table.name, streamResult.errorMessage) or
progress->reportTableFailure(table.name, streamResult.errorMessage)) so callers
can detect partial exports, and ensure setCompleted semantics reflect presence
of errors (e.g., only mark completed when no errors or provide a final
progress->setFinishedWithErrors/count). Update ExportDataProgress to expose the
chosen error-accumulation API and use that API in the !streamResult.ok branch of
exportDataToSqlFile instead of silently continuing.
- Around line 75-114: The CSV export currently ignores streamRows failures;
after calling database->streamRows (in DbDataExport::exportDataToCsvFile) check
streamResult.ok and when false report the error instead of silently continuing:
call the appropriate error reporting on ExportDataProgress (e.g.,
progress->setError or progress->addError) with a message including
streamResult.errorMessage, table.name and filename, close and/or remove the
partial file, and return early (or otherwise mark overall export failed) so
callers see the failure; ensure you include the streamResult.errorMessage from
QueryResult in the report.
In `@src/gui/exportorchestrator.cpp`:
- Around line 38-56: The race is caused by onExportComplete calling
progress_.release()/tcs_.release() while startProgressPolling's background
lambda still dereferences progress_; to fix, stop releasing raw ownership and
instead use synchronized/shared ownership and a stop signal: keep progress_ and
tcs_ alive (don't call release()), set completed_ (or request cancellation on
the provided CancellationToken) before tearing down, or capture a
shared/shared_ptr copy of progress_ inside the QtConcurrent lambda so the poller
holds ownership; ensure onExportComplete signals cancellation and waits for the
poller to exit (or relies on the token) before resetting progress_/tcs_; update
ExportOrchestrator::onExportComplete and startProgressPolling to use these
coordinated steps (avoid direct release() while poller runs).
In `@src/gui/mainwindow.cpp`:
- Around line 342-348: MainWindow is splitting the SQL script with
QString::split(";") and passing QStringList to queryPresenter->execute which
moves parsing logic into the GUI; instead change MainWindow to pass the raw
script (e.g., ui->textEdit->toPlainText()) to queryPresenter->execute (update
its signature to accept const QString& script or QByteArray and error output),
and implement the semicolon-aware SQL splitting inside the database module /
QueryExecutor (or QueryPresenter backend) so it can correctly handle semicolons
in literals/triggers; update all callers of queryPresenter->execute to the new
signature and ensure errors are still populated via the existing errors
parameter.
- Around line 400-403: The connect calls that wire
ExportOrchestrator::exportProgress and ExportOrchestrator::exportCompleted to
MainWindow::onExportProgress and MainWindow::onExportCompleted are being run on
every export and causing handlers to stack; either move these connect(...) calls
so they execute once (e.g., into MainWindow constructor where exportOrchestrator
is created/assigned) or add the Qt::UniqueConnection flag to the connect calls
to prevent duplicate connections; update both the exportProgress/exportCompleted
connects (including the ones around lines 422-425) accordingly.
In `@src/SQLIteAnalyzer.pro`:
- Around line 18-40: The qmake project file still lists removed source/header
files causing build mismatch: remove the obsolete entries database/dbquery.cpp
and gui/exportstrategy.cpp from the SOURCES block in SQLIteAnalyzer.pro and also
remove their corresponding header entries (e.g., dbquery.h and exportstrategy.h)
from the HEADERS section so the .pro matches the refactored code
(QueryExecutor/QueryExecutor.cpp and ExportOrchestrator/*.cpp now used); ensure
no other references to these filenames remain in the .pro.
---
Nitpick comments:
In `@build.ps1`:
- Around line 84-87: The script currently mutates $env:PATH by naively
prepending "$QtPath\bin;$QtPath\lib;" which can create duplicates and the
regex-based cleanup on $env:PATH can fail; change the logic in the build step
that touches $env:PATH (references: $env:PATH, $QtPath, $testExe) to instead
save $originalPath = $env:PATH before modification, build a PATH array from
$env:PATH -split ';', insert "$QtPath\bin" and "$QtPath\lib" only if they are
not already present, set $env:PATH to the joined array, run & $testExe, and
finally restore $env:PATH = $originalPath (or rebuild the PATH array to remove
only the two Qt entries reliably) to avoid leaving duplicates or polluted
session state.
In `@src/database/dbanalyzer.cpp`:
- Around line 26-51: DbAnalyzer::loadTables currently calls database->open()
unconditionally which can reopen an already-open connection; modify loadTables
to check database->isOpen() first and only call database->open() when not
already open, and ensure you only call database->close() if this method opened
the connection (track a local flag like openedHere) so you don't close a
caller-owned connection; update references inside loadTables (the open/close
logic around this->database, QueryResult handling, and final database->close())
accordingly or alternatively remove open/close from loadTables and document that
the caller must open the Database before calling loadTables on DatabaseInfo.
In `@src/database/inmemorydatabase.cpp`:
- Line 10: Remove the unnecessary call to database.setHostName("localhost") in
src/database/inmemorydatabase.cpp: since SQLite is file-based, delete the
setHostName invocation (referencing the database object and its setHostName
method) so the code only configures relevant SQLite parameters and avoids
misleading/no-op host configuration.
In `@src/database/sqlitedatabase.cpp`:
- Line 9: Remove the unnecessary call to database.setHostName("localhost") in
the SQLite connection setup: SQLite is file-based and does not use host
settings, so delete the setHostName(...) invocation (the one called on the
database variable in the SqliteDatabase/SQLite connection initialization) to
avoid misleading code and comments.
- Around line 18-21: The SqliteDatabase::open implementation unnecessarily calls
this->close() before opening; remove that redundant call and implement open to
either return true if database.isOpen() or otherwise call database.open() (use
the existing database.isOpen() and database.open() methods), so
SqliteDatabase::open no longer always closes before attempting to open.
In `@tests/CMakeLists.txt`:
- Around line 46-52: The test target currently links Qt6::Gui because
src/gui/exportorchestrator.cpp (and its header exportorchestrator.h) pulls in
src/threading/mainthread.h which depends on QGuiApplication and uses qApp with
Qt::QueuedConnection; if you want to remove Qt6::Gui from target_link_libraries
then make MainThread QtCore-only (e.g., remove or replace QGuiApplication usage
and qApp dependency and avoid Qt::QueuedConnection usage requiring a
QGuiApplication), or alternatively keep Qt6::Gui and add a clarifying comment in
CMakeLists.txt explaining that Qt6::Gui is required due to MainThread’s reliance
on QGuiApplication/qApp used by exportorchestrator.cpp.
In `@tests/test_dbexportschema.cpp`:
- Around line 71-79: The test comment reveals a documented limitation: FOREIGN
KEY and UNIQUE aren’t captured by Column during DbAnalyzer.analyze(), so
exported schemas omit them; update user-facing documentation (and optionally add
an inline TODO) to state that schema export currently preserves only PRIMARY KEY
and NOT NULL constraints — mention DbSchemaExport/exportSchema,
DbAnalyzer.analyze, and the Column struct by name so users/readers know where
the limitation originates, and leave a short note to consider extending Column
to store FK/UNIQUE in a future iteration.
🪄 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: 2a41c8de-ad23-4a82-95c3-a9ea28dae756
📒 Files selected for processing (41)
AGENTS.mdCMakeLists.txtCONTEXT.mdbuild.ps1cmake-build-debug-visual-studio-2026/CMakeFiles/clion-Debug-Visual Studio 2026-log.txtsrc/SQLIteAnalyzer.prosrc/cli/export.cppsrc/cli/script.cppsrc/database/database.cppsrc/database/database.hsrc/database/dbanalyzer.cppsrc/database/dbanalyzer.hsrc/database/dbexportdata.cppsrc/database/dbexportdata.hsrc/database/dbquery.cppsrc/database/dbquery.hsrc/database/idatabase.hsrc/database/inmemorydatabase.cppsrc/database/inmemorydatabase.hsrc/database/queryexecutor.cppsrc/database/queryexecutor.hsrc/database/queryresult.hsrc/database/sqlitedatabase.cppsrc/database/sqlitedatabase.hsrc/gui/exportorchestrator.cppsrc/gui/exportorchestrator.hsrc/gui/exportstrategy.cppsrc/gui/exportstrategy.hsrc/gui/mainwindow.cppsrc/gui/mainwindow.hsrc/gui/queryexecutionpresenter.cppsrc/gui/queryexecutionpresenter.hsrc/gui/queryresultpresenter.cppsrc/gui/queryresultpresenter.htests/CMakeLists.txttests/test_dbanalyzer.cpptests/test_dbexportdata.cpptests/test_dbexportschema.cpptests/test_exportorchestrator.cpptests/test_main.cpptests/test_queryexecutor.cpp
💤 Files with no reviewable changes (7)
- cmake-build-debug-visual-studio-2026/CMakeFiles/clion-Debug-Visual Studio 2026-log.txt
- src/database/dbquery.cpp
- src/gui/exportstrategy.h
- src/database/database.h
- src/database/dbquery.h
- src/gui/exportstrategy.cpp
- src/database/database.cpp
Comment on lines
+36
to
+38
| - **No SQL string concatenation in non-database modules.** `dbquery.h` | ||
| splitting-by-`;` lives in the database module, not in `mainwindow.cpp` or | ||
| the CLI. |
Contributor
There was a problem hiding this comment.
Update stale file reference in the SQL-splitting rule.
Line 36–Line 38 says "dbquery.h splitting-by-;", but the current execution path is centered on QueryExecutor in the database module. This rule should name the current module/symbol to avoid anchoring future changes to removed artifacts.
🤖 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 `@AGENTS.md` around lines 36 - 38, Update the stale reference in AGENTS.md to
name the current symbol/module: change the SQL-splitting rule text that mentions
"dbquery.h" to reference the database module's QueryExecutor (or the appropriate
class/function responsible for SQL splitting), and clarify that splitting-by-`;`
is implemented in the database module (QueryExecutor) not in mainwindow.cpp or
the CLI so future edits aren't anchored to removed files.
Comment on lines
+80
to
+97
| if ($LASTEXITCODE -eq 0) { | ||
| Write-Host "`nRunning tests..." | ||
| $testExe = ".\build\SQLiteQueryTests.exe" | ||
| if (Test-Path $testExe) { | ||
| $env:PATH = "$QtPath\bin;$QtPath\lib;$env:PATH" | ||
| Write-Host "Executing: $testExe" | ||
| & $testExe | ||
| $env:PATH = $env:PATH -replace [regex]::Escape("$QtPath\bin;$QtPath\lib;"), "" | ||
| 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" | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Tests only run on Windows builds, not on Linux or macOS.
The test execution block (lines 80-97) is inside the if ($IsWindows) section, but the Linux (starting line 104) and macOS (starting line 128) sections do not have corresponding test execution. This creates platform-inconsistent validation where Windows developers catch test failures but Linux/macOS builds skip verification entirely.
Add test execution to Linux and macOS sections to ensure consistent validation across all platforms. For Linux, after the build at line 107:
🧪 Proposed fix to add Linux test execution
cmake --build build --config Release --parallel $(nproc)
cmake --install build
+
+ Write-Host "`nRunning tests..."
+ $testExe = "./build/SQLiteQueryTests"
+ if (Test-Path $testExe) {
+ & $testExe
+ 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"
+ }
if ($Package) {Similar addition needed for macOS section after line 131.
🤖 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 80 - 97, Add the same test-execution logic you have
in the Windows block to the Linux and macOS sections immediately after their
build steps: set $testExe to the built test binary (use
"./build/SQLiteQueryTests" without .exe on Unix), optionally prepend
"$QtPath/bin;$QtPath/lib" to $env:PATH if $QtPath is set, log and execute &
$testExe, check $LASTEXITCODE and call exit 1 with Write-Error on failure,
restore $env:PATH (remove the injected Qt path) and emit a success message on
pass, and emit a warning if the test binary is not found; reference the same
symbols ($testExe, $env:PATH, $QtPath, $LASTEXITCODE) so the logic mirrors the
Windows block exactly.
Comment on lines
+94
to
+96
| else { | ||
| Write-Warning "Test executable not found at: $testExe" | ||
| } |
Contributor
There was a problem hiding this comment.
Missing test executable should fail the build, not just warn.
Line 95 only emits a warning if the test executable is not found. Based on the learnings, "Before any commit, the build must pass by running: pwsh -NoProfile -File build.ps1." Since tests are now part of the build process (per PR objectives to add comprehensive test coverage), a missing test executable likely indicates a build configuration error that should fail the build rather than silently continue.
🛡️ Proposed fix to fail on missing tests
else {
- Write-Warning "Test executable not found at: $testExe"
+ Write-Error "Test executable not found at: $testExe"
+ exit 1
}
}📝 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
| else { | |
| Write-Warning "Test executable not found at: $testExe" | |
| } | |
| else { | |
| 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 94 - 96, The else branch that currently calls
Write-Warning when the test executable referenced by $testExe is missing should
instead fail the build: replace the warning with a failing action (e.g.,
Write-Error with the same message and then exit 1 or throw) inside the else
block so the script terminates with a non-zero exit code when $testExe is not
found; update the branch that checks $testExe in build.ps1 to stop execution
rather than continue.
|
|
||
| ## User-facing actions | ||
|
|
||
| - **Query** — a single SQL statement the user wrote and wants executed. |
Contributor
There was a problem hiding this comment.
Tighten wording for the Query definition.
Line 25 reads awkwardly (“wants executed”). Rephrase to “wants to execute” or “wants to be executed” for clarity.
🧰 Tools
🪛 LanguageTool
[style] ~25-~25: The double modal “wants executed” is nonstandard (only accepted in certain dialects). Consider “to be executed”.
Context: ... SQL statement the user wrote and wants executed. - Script — a sequence of Queries l...
(NEEDS_FIXED)
🤖 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` at line 25, Update the wording of the "Query" definition to use
clearer phrasing: replace "a single SQL statement the user wrote and wants
executed." with either "a single SQL statement the user wrote and wants to
execute." or "a single SQL statement the user wrote and wants to be executed."
so the definition for "Query" reads cleanly and unambiguously.
Source: Linters/SAST tools
Comment on lines
+55
to
+56
| - **ExportStrategy** — abstract DataExporter workflow (`SqlExportStrategy`, | ||
| `CsvExportStrategy`). Two adapters = real seam. |
Contributor
There was a problem hiding this comment.
Remove stale ExportStrategy seam from the context vocabulary.
Line 55–Line 56 still define ExportStrategy, but this refactor replaced strategy-based export entrypoints with ExportOrchestrator::exportToSql() / exportToCsv(). Keeping this seam in CONTEXT.md will misdirect future module naming and architecture decisions.
Suggested doc update
-- **ExportStrategy** — abstract DataExporter workflow (`SqlExportStrategy`,
- `CsvExportStrategy`). Two adapters = real seam.
+`ExportOrchestrator` exposes direct export flows (`exportToSql`,
+`exportToCsv`) and coordinates async export progress/cancellation.📝 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
| - **ExportStrategy** — abstract DataExporter workflow (`SqlExportStrategy`, | |
| `CsvExportStrategy`). Two adapters = real seam. | |
| `ExportOrchestrator` exposes direct export flows (`exportToSql`, | |
| `exportToCsv`) and coordinates async export progress/cancellation. |
🤖 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 - 56, Remove the stale "ExportStrategy" entry
from CONTEXT.md and update the vocabulary to reference the new
orchestrator-based API: replace mentions of ExportStrategy (and any references
to SqlExportStrategy/CsvExportStrategy) with ExportOrchestrator and its methods
exportToSql() and exportToCsv(), ensuring the doc reflects the current design
where ExportOrchestrator provides export entrypoints instead of a strategy seam.
Comment on lines
+25
to
+33
| void QueryResultPresenter::present(const QList<QueryResult> &results) { | ||
| const QRect widgetRect = this->widget->geometry(); | ||
| const int width = widgetRect.width(); | ||
| const int height = widgetRect.height(); | ||
|
|
||
| int yOffset = 0; | ||
| int count = 0; | ||
|
|
||
| for (const auto &result: results) { |
Contributor
There was a problem hiding this comment.
present() should clear previous render state before appending new tables.
Line 25 starts rendering without clearing prior tableViews. Since QueryExecutionPresenter::execute forwards results directly each run, repeated executions can stack stale tables and retain unnecessary widgets.
🤖 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 25 - 33,
QueryResultPresenter::present currently appends new table widgets on each call
without clearing prior state; before iterating results, remove and delete any
existing widgets stored in tableViews from this->widget (or its layout), clear
the tableViews container, and reset any related offsets/counters (yOffset,
count) so the new render starts fresh; ensure you both remove the widgets from
the parent (or layout) and delete them to avoid leaks, then proceed with the
existing loop to create and store new table views.
Comment on lines
+40
to
+57
| auto model = std::make_unique<QStandardItemModel>( | ||
| static_cast<int>(result.rows.size()), | ||
| static_cast<int>(result.columns.size()), | ||
| container.get()); | ||
|
|
||
| for (int col = 0; col < result.columns.size(); ++col) { | ||
| model->setHorizontalHeaderItem(col, new QStandardItem(result.columns.at(col))); | ||
| } | ||
| for (int row = 0; row < result.rows.size(); ++row) { | ||
| const auto &values = result.rows.at(row).values; | ||
| for (int col = 0; col < result.columns.size(); ++col) { | ||
| auto *item = new QStandardItem(values.at(col).toString()); | ||
| model->setItem(row, col, item); | ||
| } | ||
| } | ||
|
|
||
| const auto tablePtr = new QTableView(container.get()); | ||
| tablePtr->setModel(model.release()); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
repo_root="$(pwd)"
# Show file with line numbers around the snippet and also earlier where clear() is defined/used
file="src/gui/queryresultpresenter.cpp"
echo "== File exists =="; ls -l "$file" || true
echo "== Lines 1-120 =="
sed -n '1,120p' "$file" | cat -n
echo "== Lines 120-260 =="
sed -n '120,260p' "$file" | cat -n
echo "== Search for clear() usage in the file =="
rg -n "clear\\(\\)|\\bclear\\b" "$file" || true
echo "== Search for QTableView/QStandardItemModel creation and parenting in the file =="
rg -n "QTableView|QStandardItemModel|setModel\\(|setParent\\(|make_unique<QStandardItemModel" "$file" || trueRepository: christianhelle/sqlitequery
Length of output: 4452
🏁 Script executed:
#!/bin/bash
set -euo pipefail
repo_root="$(pwd)"
file="src/gui/queryresultpresenter.cpp"
echo "== Lines 1-220 =="
sed -n '1,220p' "$file" | cat -n
echo "== Search for clear() usage =="
rg -n "clear\\(\\)|\\bclear\\b" "$file" || true
echo "== Search for QTableView/QStandardItemModel creation/parenting =="
rg -n "QTableView|QStandardItemModel|setModel\\(|new QTableView|make_unique<QStandardItemModel|setItem\\(|setHorizontalHeaderItem\\(" "$file" || trueRepository: christianhelle/sqlitequery
Length of output: 4588
Fix QueryResultPresenter::clear() model lifetime: QStandardItemModels aren’t deleted when clearing.
present() creates QStandardItemModel with container.get() as the QObject parent, then releases ownership to QTableView via setModel(model.release()). clear() deletes only the QTableView instances (qDeleteAll(this->tableViews...)) but never deletes the models; since they’re children of container, they survive across clears/repeated executions until container is destroyed.
Suggested fix
- auto model = std::make_unique<QStandardItemModel>(
- static_cast<int>(result.rows.size()),
- static_cast<int>(result.columns.size()),
- container.get());
+ const auto tablePtr = new QTableView(container.get());
+ auto model = std::make_unique<QStandardItemModel>(
+ static_cast<int>(result.rows.size()),
+ static_cast<int>(result.columns.size()),
+ tablePtr);
@@
- const auto tablePtr = new QTableView(container.get());
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 40 - 57, present() creates a
QStandardItemModel with container.get() as parent then does model.release() and
setModel(...), so when clear() deletes only QTableView objects the models
(children of container) leak across runs; fix by making the model a child of the
QTableView so it is deleted with the view: create the QTableView (tablePtr)
first, then construct the QStandardItemModel with tablePtr as its parent (or
call model->setParent(tablePtr) before model.release()), and then call
tablePtr->setModel(model.release()); also ensure clear() continues to
qDeleteAll(this->tableViews...) so views and their child models are destroyed
together (referencing present(), clear(), the QStandardItemModel construction
and tablePtr->setModel calls).
Comment on lines
+69
to
+87
| void QueryResultPresenter::presentToView(QTableView *view, const QueryResult &result) { | ||
| auto model = std::make_unique<QStandardItemModel>( | ||
| static_cast<int>(result.rows.size()), | ||
| static_cast<int>(result.columns.size()), | ||
| view); | ||
|
|
||
| for (int col = 0; col < result.columns.size(); ++col) { | ||
| model->setHorizontalHeaderItem(col, new QStandardItem(result.columns.at(col))); | ||
| } | ||
| for (int row = 0; row < result.rows.size(); ++row) { | ||
| const auto &values = result.rows.at(row).values; | ||
| for (int col = 0; col < result.columns.size(); ++col) { | ||
| auto *item = new QStandardItem(values.at(col).toString()); | ||
| model->setItem(row, col, item); | ||
| } | ||
| } | ||
|
|
||
| view->setModel(model.release()); | ||
| view->setSortingEnabled(true); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the file and print the relevant function with line numbers
sed -n '1,200p' src/gui/queryresultpresenter.cpp | cat -n
# Print just the surrounding lines of the suspected section (for easier review)
sed -n '50,130p' src/gui/queryresultpresenter.cpp | cat -n
# Find all uses of presentToView in the repo
rg -n "presentToView\\(" -S .
# Check whether there is any model reuse / explicit deletion nearby
rg -n "setModel\\(|QStandardItemModel|model\\.release\\(" src/gui/queryresultpresenter.cppRepository: christianhelle/sqlitequery
Length of output: 6324
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show full call sites around the referenced line numbers
sed -n '430,520p' src/gui/mainwindow.cpp | cat -n
sed -n '1,120p' src/gui/queryexecutionpresenter.cpp | cat -n
# Confirm the header declaration
sed -n '1,120p' src/gui/queryresultpresenter.h | cat -nRepository: christianhelle/sqlitequery
Length of output: 4655
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for any other model handling on ui->tableView
rg -n "tableView" src/gui -S
rg -n "setModel\\(" src/gui -S
# Look for any explicit deletion/reset around table models
rg -n "delete .*model|destroy\\(\\)|setModel\\(nullptr|model\\(\\)" src/gui -S
# Also inspect queryresultpresenter.cpp to see if clear() handles models for single view
sed -n '1,120p' src/gui/queryresultpresenter.cpp | cat -nRepository: christianhelle/sqlitequery
Length of output: 810
Repeated QueryResultPresenter::presentToView() calls accumulate old QStandardItemModel instances on the same QTableView.
presentToView() creates a new QStandardItemModel with view as parent and calls view->setModel(model.release()); setModel() only replaces the pointer—it doesn’t delete the previous model—so the old models remain alive (until view teardown), accumulating across refreshes.
🤖 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 69 - 87, presentToView
currently creates a new QStandardItemModel and releases it into view->setModel()
while leaving previous models alive; fix by (1) constructing the
QStandardItemModel without parenting it to the view (use nullptr parent when
creating the model in QueryResultPresenter::presentToView) and (2) before
calling view->setModel(model.release()), check view->model() and if non-null
call view->setModel(nullptr) then delete the old model pointer to free the
previous QStandardItemModel; this ensures no accumulation of orphaned models
when presentToView is called repeatedly.
| set(CMAKE_CXX_STANDARD 17) | ||
| set(CMAKE_CXX_STANDARD_REQUIRED ON) | ||
|
|
||
| find_package(Qt6 REQUIRED COMPONENTS Core Sql) |
Contributor
There was a problem hiding this comment.
Qt version mismatch: tests hardcode Qt6, but main project supports Qt5/Qt6.
The test configuration requires Qt6 specifically, while the main CMakeLists.txt supports both Qt5 and Qt6 via find_package(QT NAMES Qt5 Qt6 REQUIRED COMPONENTS Core). This prevents running tests on Qt5 environments and breaks the project's Qt version flexibility.
🔧 Proposed fix to match main project Qt flexibility
-find_package(Qt6 REQUIRED COMPONENTS Core Sql)
+find_package(QT NAMES Qt5 Qt6 REQUIRED COMPONENTS Core)
+find_package(Qt${QT_VERSION_MAJOR} REQUIRED COMPONENTS Core Sql)Then update the link libraries section:
target_link_libraries(${PROJECT_NAME} PRIVATE
- Qt6::Core
- Qt6::Gui
- Qt6::Sql
+ Qt${QT_VERSION_MAJOR}::Core
+ Qt${QT_VERSION_MAJOR}::Gui
+ Qt${QT_VERSION_MAJOR}::Sql
GTest::gtest_main
GTest::gtest
)🤖 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 `@tests/CMakeLists.txt` at line 7, Replace the hardcoded Qt6 find_package in
tests' CMakeLists.txt with the same flexible pattern used in the main
CMakeLists: use find_package(QT NAMES Qt5 Qt6 REQUIRED COMPONENTS Core Sql) to
allow either Qt5 or Qt6, and update any target_link_libraries references (e.g.,
the test target that currently links Qt::Core/Qt::Sql or Qt6::Core/Qt6::Sql) to
use the imported targets produced by that find_package (such as
Qt${QT_VERSION_MAJOR}::Core and Qt${QT_VERSION_MAJOR}::Sql or the generic QT
targets from find_package) so linking works for both Qt5 and Qt6.
Comment on lines
+31
to
+42
| set(PROD_SRCS | ||
| ../src/database/sqlitedatabase.cpp | ||
| ../src/database/inmemorydatabase.cpp | ||
| ../src/database/queryexecutor.cpp | ||
| ../src/database/dbanalyzer.cpp | ||
| ../src/database/dbexportdata.cpp | ||
| ../src/database/dbexportschema.cpp | ||
| ../src/database/dbexport.cpp | ||
| ../src/database/progress.h | ||
| ../src/threading/cancellation.cpp | ||
| ../src/gui/exportorchestrator.cpp | ||
| ) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if progress.h has implementation or Q_OBJECT
# Check for implementation code or Q_OBJECT macro
rg -n 'Q_OBJECT|^\s*\{|^\s*\}' src/database/progress.hRepository: christianhelle/sqlitequery
Length of output: 107
Remove ../src/database/progress.h from PROD_SRCS
progress.h is a header-only file and contains no Qt meta-object triggers (no Q_OBJECT), so it shouldn’t be compiled as a source.
🧹 Proposed cleanup
../src/database/dbexportdata.cpp
../src/database/dbexportschema.cpp
../src/database/dbexport.cpp
- ../src/database/progress.h
../src/threading/cancellation.cpp
../src/gui/exportorchestrator.cpp
)📝 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
| set(PROD_SRCS | |
| ../src/database/sqlitedatabase.cpp | |
| ../src/database/inmemorydatabase.cpp | |
| ../src/database/queryexecutor.cpp | |
| ../src/database/dbanalyzer.cpp | |
| ../src/database/dbexportdata.cpp | |
| ../src/database/dbexportschema.cpp | |
| ../src/database/dbexport.cpp | |
| ../src/database/progress.h | |
| ../src/threading/cancellation.cpp | |
| ../src/gui/exportorchestrator.cpp | |
| ) | |
| set(PROD_SRCS | |
| ../src/database/sqlitedatabase.cpp | |
| ../src/database/inmemorydatabase.cpp | |
| ../src/database/queryexecutor.cpp | |
| ../src/database/dbanalyzer.cpp | |
| ../src/database/dbexportdata.cpp | |
| ../src/database/dbexportschema.cpp | |
| ../src/database/dbexport.cpp | |
| ../src/threading/cancellation.cpp | |
| ../src/gui/exportorchestrator.cpp | |
| ) |
🤖 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 `@tests/CMakeLists.txt` around lines 31 - 42, Remove the header-only progress.h
from the PROD_SRCS list so it is not treated as a compilable source: edit the
CMake variable PROD_SRCS and delete the entry "../src/database/progress.h"
(leave other entries intact) so that only .cpp files are compiled; if progress.h
needs to be available to targets, ensure the directory is reachable via
target_include_directories or target_include_directories for the target that
uses it.
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.
This pull request introduces a significant refactor to the database layer, establishes clear architectural conventions and domain vocabulary, and improves build and test integration. The changes focus on replacing the old concrete
Databaseclass with a seam-based abstraction (IDatabase), introducing production and test adapters, and aligning the codebase and documentation with new architectural guidelines.Key highlights:
IDatabaseinterface, withSqliteDatabaseandInMemoryDatabaseas concrete implementations.AGENTS.md,CONTEXT.md) are added to codify conventions and shared vocabulary.Most important changes:
1. Database Layer Refactor
Databaseclass with an abstractIDatabaseinterface and providedSqliteDatabase(production) andInMemoryDatabase(test) adapters. All modules that previously depended onDatabasenow useIDatabase*, enforcing a proper seam. (src/database/database.h,src/database/database.cpp,src/database/idatabase.h,src/database/sqlitedatabase.h,src/database/sqlitedatabase.cpp,src/database/inmemorydatabase.h,src/database/inmemorydatabase.cpp, [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14] [15]2. Architectural Documentation and Conventions
AGENTS.mdto document build requirements, naming conventions, module boundaries, and banned vocabulary. (AGENTS.md, AGENTS.mdR1-R53)CONTEXT.mdto define domain vocabulary, module responsibilities, and architectural seams, ensuring shared understanding across the codebase. (CONTEXT.md, CONTEXT.mdR1-R61)3. Project Structure and Build System
CMakeLists.txtand.profiles to reflect the new module structure, source/header file organization, and to add test subdirectory for automated test integration. (CMakeLists.txt,src/SQLIteAnalyzer.pro, [1] [2] [3] [4]build.ps1to run tests automatically after building, improving CI reliability. (build.ps1, build.ps1R80-R98)4. Code Modernization and Clean-up
Databaseimplementation and directQSqlDatabaseusage outside of the database module, enforcing architectural rules. (src/database/database.h,src/database/database.cpp, [1] [2]5. Adapter and Presenter Modules
QueryExecutionPresenter,QueryResultPresenter) and updated the project files to include them, supporting the separation of GUI and logic as per new conventions. (src/gui/queryexecutionpresenter.h,src/gui/queryexecutionpresenter.cpp,src/gui/queryresultpresenter.h,src/gui/queryresultpresenter.cpp, [1] [2]These changes collectively move the project toward a more maintainable, testable, and clearly-structured architecture, with explicit seams and adapters, and a shared vocabulary for all contributors.