From 18f8bf99b232b2e9e12630a6abe0473555d57671 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 27 Dec 2025 03:10:27 +0000 Subject: [PATCH] Fix code review issues: matrix parsing, Qt6 compatibility, and performance optimizations Co-authored-by: johndoe6345789 <224850594+johndoe6345789@users.noreply.github.com> --- include/backends/ContainerBackend.h | 4 ++++ include/gui/MainWindow.h | 1 + src/backends/ContainerBackend.cpp | 11 +++++++---- src/core/MatrixStrategy.cpp | 3 ++- src/core/WorkflowParser.cpp | 21 +++++++++++++++++++-- src/gui/MainWindow.cpp | 8 ++++---- 6 files changed, 37 insertions(+), 11 deletions(-) diff --git a/include/backends/ContainerBackend.h b/include/backends/ContainerBackend.h index 5ec6211..535e1db 100644 --- a/include/backends/ContainerBackend.h +++ b/include/backends/ContainerBackend.h @@ -23,6 +23,10 @@ public: void cleanup() override; private: + static constexpr int STEP_TIMEOUT_MS = 300000; // 5 minutes + static constexpr int PREPARE_TIMEOUT_MS = 60000; // 1 minute + static constexpr int CLEANUP_TIMEOUT_MS = 30000; // 30 seconds + QString m_containerId; QString m_containerRuntime; // "docker" or "podman" diff --git a/include/gui/MainWindow.h b/include/gui/MainWindow.h index f9f0d56..959cc2c 100644 --- a/include/gui/MainWindow.h +++ b/include/gui/MainWindow.h @@ -46,6 +46,7 @@ private: std::unique_ptr m_repoManager; std::unique_ptr m_executor; + std::unique_ptr m_parser; }; } // namespace gui diff --git a/src/backends/ContainerBackend.cpp b/src/backends/ContainerBackend.cpp index 032a1cc..b1073de 100644 --- a/src/backends/ContainerBackend.cpp +++ b/src/backends/ContainerBackend.cpp @@ -26,11 +26,14 @@ bool ContainerBackend::executeStep(const core::WorkflowStep& step, if (!step.run.isEmpty()) { QProcess process; QStringList args; - args << "exec" << m_containerId << "sh" << "-c" << step.run; + + // Use specified shell or default to sh + QString shell = step.shell.isEmpty() ? "sh" : step.shell; + args << "exec" << m_containerId << shell << "-c" << step.run; process.start(m_containerRuntime, args); - if (!process.waitForFinished(300000)) { // 5 minutes + if (!process.waitForFinished(STEP_TIMEOUT_MS)) { emit error("Step execution timeout"); return false; } @@ -61,7 +64,7 @@ bool ContainerBackend::prepareEnvironment(const QString& runsOn) { process.start(m_containerRuntime, args); - if (!process.waitForFinished(60000)) { + if (!process.waitForFinished(PREPARE_TIMEOUT_MS)) { emit error("Container creation timeout"); return false; } @@ -83,7 +86,7 @@ void ContainerBackend::cleanup() { args << "rm" << "-f" << m_containerId; process.start(m_containerRuntime, args); - process.waitForFinished(30000); + process.waitForFinished(CLEANUP_TIMEOUT_MS); m_containerId.clear(); } diff --git a/src/core/MatrixStrategy.cpp b/src/core/MatrixStrategy.cpp index 937af81..a17d9ad 100644 --- a/src/core/MatrixStrategy.cpp +++ b/src/core/MatrixStrategy.cpp @@ -1,5 +1,6 @@ #include "core/MatrixStrategy.h" #include "core/WorkflowParser.h" +#include #include namespace gwt { @@ -66,7 +67,7 @@ QList MatrixStrategy::generateCombinations(const QVariantMap& matri for (const QString& key : keys) { QVariant value = matrix[key]; - if (value.type() == QVariant::List) { + if (value.typeId() == QMetaType::QVariantList) { valueLists << value.toList(); } else { valueLists << QVariantList{value}; diff --git a/src/core/WorkflowParser.cpp b/src/core/WorkflowParser.cpp index 391bbab..899833a 100644 --- a/src/core/WorkflowParser.cpp +++ b/src/core/WorkflowParser.cpp @@ -118,8 +118,25 @@ Workflow WorkflowParser::parse(const QString& filePath) { if (jobNode["strategy"]) { YAML::Node strategyNode = jobNode["strategy"]; if (strategyNode["matrix"]) { - // Store as QVariantMap for processing by MatrixStrategy - job.strategy["matrix"] = "present"; + YAML::Node matrixNode = strategyNode["matrix"]; + QVariantMap matrixMap; + + for (auto it = matrixNode.begin(); it != matrixNode.end(); ++it) { + QString key = QString::fromStdString(it->first.as()); + YAML::Node valueNode = it->second; + + if (valueNode.IsSequence()) { + QVariantList values; + for (size_t i = 0; i < valueNode.size(); ++i) { + values << QString::fromStdString(valueNode[i].as()); + } + matrixMap[key] = values; + } else { + matrixMap[key] = QString::fromStdString(valueNode.as()); + } + } + + job.strategy["matrix"] = matrixMap; } } diff --git a/src/gui/MainWindow.cpp b/src/gui/MainWindow.cpp index dba5206..609d380 100644 --- a/src/gui/MainWindow.cpp +++ b/src/gui/MainWindow.cpp @@ -20,6 +20,7 @@ MainWindow::MainWindow(QWidget* parent) : QMainWindow(parent) , m_repoManager(std::make_unique()) , m_executor(std::make_unique()) + , m_parser(std::make_unique()) { setupUI(); loadRepositories(); @@ -164,12 +165,11 @@ void MainWindow::onRunWorkflow() { m_outputView->append("\n=== Running workflow: " + workflowPath + " ===\n"); // Parse and execute - core::WorkflowParser parser; - core::Workflow workflow = parser.parse(workflowPath); + core::Workflow workflow = m_parser->parse(workflowPath); - if (parser.hasErrors()) { + if (m_parser->hasErrors()) { m_outputView->append("Parsing errors:"); - for (const QString& error : parser.getErrors()) { + for (const QString& error : m_parser->getErrors()) { m_outputView->append(" " + error); } return;