From b2f4818405446b33e11a4869f04d29ea98d3da45 Mon Sep 17 00:00:00 2001 From: Bill Date: Mon, 16 Feb 2026 17:26:23 -0700 Subject: [PATCH] Add step436 review comparison view and tests --- editor/CMakeLists.txt | 9 ++ editor/src/ReviewComparisonView.h | 118 ++++++++++++++++++++ editor/tests/step436_test.cpp | 173 ++++++++++++++++++++++++++++++ progress.md | 45 ++++++++ 4 files changed, 345 insertions(+) create mode 100644 editor/src/ReviewComparisonView.h create mode 100644 editor/tests/step436_test.cpp diff --git a/editor/CMakeLists.txt b/editor/CMakeLists.txt index d0ef1f5..2b37068 100644 --- a/editor/CMakeLists.txt +++ b/editor/CMakeLists.txt @@ -2803,4 +2803,13 @@ target_link_libraries(step435_test PRIVATE tree_sitter_javascript tree_sitter_typescript tree_sitter_java tree_sitter_rust tree_sitter_go) +add_executable(step436_test tests/step436_test.cpp) +target_include_directories(step436_test PRIVATE src) +target_link_libraries(step436_test PRIVATE + nlohmann_json::nlohmann_json + unofficial::tree-sitter::tree-sitter + tree_sitter_python tree_sitter_cpp tree_sitter_elisp + tree_sitter_javascript tree_sitter_typescript + tree_sitter_java tree_sitter_rust tree_sitter_go) + # Step 12: Dear ImGui shell scaffolding created (main.cpp exists but not built due to dependencies) diff --git a/editor/src/ReviewComparisonView.h b/editor/src/ReviewComparisonView.h new file mode 100644 index 0000000..7528cb6 --- /dev/null +++ b/editor/src/ReviewComparisonView.h @@ -0,0 +1,118 @@ +#pragma once + +#include "WorkflowTaskDetail.h" + +#include +#include +#include + +struct ReviewDiffLine { + char kind = ' '; // ' ' unchanged, '-' removed, '+' added, '~' modified + std::string left; + std::string right; + std::string note; +}; + +struct ReviewComparisonModel { + std::string itemId; + std::string title; + std::string status; + std::string leftLabel; + std::string rightLabel; + std::vector diff; + std::vector changeNotes; + bool canApprove = false; + bool canReject = false; + std::string approveShortcut; + std::string rejectShortcut; +}; + +class ReviewComparisonView { +public: + static std::optional build(const WorkflowState& wf, + const std::string& itemId) { + auto detail = WorkflowTaskDetail::build(wf, itemId); + if (!detail.has_value()) return std::nullopt; + + ReviewComparisonModel m; + m.itemId = detail->itemId; + m.title = detail->title; + m.status = detail->status; + m.leftLabel = "skeleton"; + m.rightLabel = "generated"; + m.diff = buildDiffLines(detail->skeletonCode, detail->generatedCode); + m.changeNotes = buildChangeNotes(*detail); + m.canApprove = (detail->status == WI_REVIEW); + m.canReject = (detail->status == WI_REVIEW); + m.approveShortcut = approveShortcut(); + m.rejectShortcut = rejectShortcut(); + return m; + } + + static bool approve(WorkflowState& wf, const std::string& itemId, + const std::string& reviewer = "human") { + return WorkflowTaskDetail::approve(wf, itemId, reviewer); + } + + static bool reject(WorkflowState& wf, const std::string& itemId, + const std::string& feedback, + const std::string& reviewer = "human") { + return WorkflowTaskDetail::reject(wf, itemId, feedback, reviewer); + } + + static std::string approveShortcut() { return "Ctrl+Shift+A"; } + + static std::string rejectShortcut() { return "Ctrl+Shift+R"; } + +private: + static std::vector splitLines(const std::string& text) { + std::vector lines; + std::string cur; + for (char c : text) { + if (c == '\n') { + lines.push_back(cur); + cur.clear(); + } else { + cur.push_back(c); + } + } + if (!cur.empty()) lines.push_back(cur); + return lines; + } + + static std::vector buildDiffLines(const std::string& left, + const std::string& right) { + auto l = splitLines(left); + auto r = splitLines(right); + size_t n = l.size() > r.size() ? l.size() : r.size(); + std::vector out; + for (size_t i = 0; i < n; ++i) { + std::string ls = i < l.size() ? l[i] : ""; + std::string rs = i < r.size() ? r[i] : ""; + if (i >= l.size()) { + out.push_back({'+', "", rs, "added"}); + } else if (i >= r.size()) { + out.push_back({'-', ls, "", "removed"}); + } else if (ls == rs) { + out.push_back({' ', ls, rs, "unchanged"}); + } else { + out.push_back({'~', ls, rs, "modified"}); + } + } + return out; + } + + static std::vector buildChangeNotes(const TaskDetailView& detail) { + std::vector notes; + if (!detail.routingExplanation.empty()) { + notes.push_back("routing: " + detail.routingExplanation); + } + if (!detail.rejectionFeedback.empty()) { + notes.push_back("latest-feedback: " + detail.rejectionFeedback); + } + for (const auto& t : detail.annotationTags) { + notes.push_back("annotation: " + t); + } + return notes; + } +}; diff --git a/editor/tests/step436_test.cpp b/editor/tests/step436_test.cpp new file mode 100644 index 0000000..c85bc3e --- /dev/null +++ b/editor/tests/step436_test.cpp @@ -0,0 +1,173 @@ +// Step 436: Review Comparison View Tests (12 tests) + +#include "ReviewComparisonView.h" + +#include + +static int passed = 0, failed = 0; +#define TEST(name) { std::cout << " " << #name << "... "; } +#define PASS() { std::cout << "PASS\n"; ++passed; } +#define FAIL(msg) { std::cout << "FAIL: " << msg << "\n"; ++failed; } +#define CHECK(cond, msg) if (!(cond)) { FAIL(msg); return; } else {} + +static WorkflowState sampleReviewState() { + WorkflowState wf("review"); + WorkItem wi; + wi.id = "r1"; + wi.nodeId = "node-review"; + wi.nodeName = "computePath"; + wi.bufferId = "src/path.cpp"; + wi.status = WI_REVIEW; + wi.workerType = "llm"; + wi.priority = "high"; + wi.contextWidth = "file"; + wi.reviewRequired = true; + wi.result.generatedCode = "void computePath() {\n int x = 1;\n}\n"; + wi.rejectionFeedback = "add bounds check"; + wf.queue.enqueue(wi); + return wf; +} + +void test_build_returns_model_for_existing_item() { + TEST(build_returns_model_for_existing_item); + auto wf = sampleReviewState(); + auto model = ReviewComparisonView::build(wf, "r1"); + CHECK(model.has_value(), "model should exist"); + CHECK(model->itemId == "r1", "item id mismatch"); + PASS(); +} + +void test_build_returns_null_for_missing_item() { + TEST(build_returns_null_for_missing_item); + auto wf = sampleReviewState(); + auto model = ReviewComparisonView::build(wf, "missing"); + CHECK(!model.has_value(), "missing item should return nullopt"); + PASS(); +} + +void test_diff_contains_modified_line_entries() { + TEST(diff_contains_modified_line_entries); + auto wf = sampleReviewState(); + auto model = ReviewComparisonView::build(wf, "r1"); + CHECK(model.has_value(), "expected model"); + bool hasModified = false; + for (const auto& d : model->diff) if (d.kind == '~') hasModified = true; + CHECK(hasModified, "diff should contain modified line"); + PASS(); +} + +void test_review_status_enables_approve_and_reject() { + TEST(review_status_enables_approve_and_reject); + auto wf = sampleReviewState(); + auto model = ReviewComparisonView::build(wf, "r1"); + CHECK(model.has_value(), "expected model"); + CHECK(model->canApprove, "review item should allow approve"); + CHECK(model->canReject, "review item should allow reject"); + PASS(); +} + +void test_shortcuts_match_spec() { + TEST(shortcuts_match_spec); + CHECK(ReviewComparisonView::approveShortcut() == "Ctrl+Shift+A", "approve shortcut mismatch"); + CHECK(ReviewComparisonView::rejectShortcut() == "Ctrl+Shift+R", "reject shortcut mismatch"); + PASS(); +} + +void test_model_includes_shortcuts() { + TEST(model_includes_shortcuts); + auto wf = sampleReviewState(); + auto model = ReviewComparisonView::build(wf, "r1"); + CHECK(model.has_value(), "expected model"); + CHECK(model->approveShortcut == "Ctrl+Shift+A", "approve shortcut missing"); + CHECK(model->rejectShortcut == "Ctrl+Shift+R", "reject shortcut missing"); + PASS(); +} + +void test_change_notes_include_routing() { + TEST(change_notes_include_routing); + auto wf = sampleReviewState(); + auto model = ReviewComparisonView::build(wf, "r1"); + CHECK(model.has_value(), "expected model"); + bool hasRouting = false; + for (const auto& n : model->changeNotes) if (n.find("routing:") == 0) hasRouting = true; + CHECK(hasRouting, "routing note expected"); + PASS(); +} + +void test_change_notes_include_rejection_feedback() { + TEST(change_notes_include_rejection_feedback); + auto wf = sampleReviewState(); + auto model = ReviewComparisonView::build(wf, "r1"); + CHECK(model.has_value(), "expected model"); + bool hasFeedback = false; + for (const auto& n : model->changeNotes) if (n.find("latest-feedback:") == 0) hasFeedback = true; + CHECK(hasFeedback, "feedback note expected"); + PASS(); +} + +void test_approve_transitions_item_to_complete() { + TEST(approve_transitions_item_to_complete); + auto wf = sampleReviewState(); + bool ok = ReviewComparisonView::approve(wf, "r1", "architect"); + CHECK(ok, "approve should succeed"); + auto updated = wf.queue.getItem("r1"); + CHECK(updated.has_value(), "item should exist"); + CHECK(updated->status == WI_COMPLETE, "status should be complete"); + PASS(); +} + +void test_reject_transitions_item_to_ready() { + TEST(reject_transitions_item_to_ready); + auto wf = sampleReviewState(); + bool ok = ReviewComparisonView::reject(wf, "r1", "needs tests", "architect"); + CHECK(ok, "reject should succeed"); + auto updated = wf.queue.getItem("r1"); + CHECK(updated.has_value(), "item should exist"); + CHECK(updated->status == WI_READY, "status should be ready after rejection"); + PASS(); +} + +void test_reject_requires_feedback() { + TEST(reject_requires_feedback); + auto wf = sampleReviewState(); + bool ok = ReviewComparisonView::reject(wf, "r1", "", "architect"); + CHECK(!ok, "reject should fail without feedback"); + PASS(); +} + +void test_non_review_item_disables_actions() { + TEST(non_review_item_disables_actions); + auto wf = sampleReviewState(); + auto item = wf.queue.getItem("r1"); + CHECK(item.has_value(), "item missing"); + WorkItem updated = *item; + updated.status = WI_COMPLETE; + CHECK(wf.queue.updateItem("r1", updated), "update failed"); + + auto model = ReviewComparisonView::build(wf, "r1"); + CHECK(model.has_value(), "expected model"); + CHECK(!model->canApprove, "approve should be disabled"); + CHECK(!model->canReject, "reject should be disabled"); + PASS(); +} + +int main() { + std::cout << "Step 436: Review Comparison View Tests\n"; + + test_build_returns_model_for_existing_item(); // 1 + test_build_returns_null_for_missing_item(); // 2 + test_diff_contains_modified_line_entries(); // 3 + test_review_status_enables_approve_and_reject(); // 4 + test_shortcuts_match_spec(); // 5 + test_model_includes_shortcuts(); // 6 + test_change_notes_include_routing(); // 7 + test_change_notes_include_rejection_feedback(); // 8 + test_approve_transitions_item_to_complete(); // 9 + test_reject_transitions_item_to_ready(); // 10 + test_reject_requires_feedback(); // 11 + test_non_review_item_disables_actions(); // 12 + + std::cout << "\nResults: " << passed << "/" << (passed + failed) + << " passed\n"; + return failed == 0 ? 0 : 1; +} diff --git a/progress.md b/progress.md index 6efdb18..c95e616 100644 --- a/progress.md +++ b/progress.md @@ -5129,6 +5129,51 @@ state into editor views, with status icon/color mapping and task-detail routes. - `editor/src/MCPServer.h` (`1940` > `600`) - `editor/src/HeadlessAgentRPCHandler.h` (`2768` > `600`) +### Step 436: Review Comparison View +**Status:** PASS (12/12 tests) + +Added a review-comparison model that produces side-by-side diff rows and +integrates approve/reject actions with keyboard shortcut metadata. + +**Files created:** +- `editor/src/ReviewComparisonView.h` — review diff model support: + - workflow item -> comparison model projection + - skeleton/generated side labels + - line-oriented diff rows (added/removed/modified/unchanged) + - contextual change notes (routing, feedback, annotations) + - action availability flags keyed to review status + - approve/reject wrappers delegated to workflow detail logic + - keyboard shortcut constants (`Ctrl+Shift+A`, `Ctrl+Shift+R`) +- `editor/tests/step436_test.cpp` — 12 tests covering: + 1. model generation for existing item + 2. missing-item null handling + 3. modified-line diff detection + 4. review-state action enablement + 5. shortcut constant mapping + 6. shortcut propagation into model + 7. routing note inclusion + 8. rejection-feedback note inclusion + 9. approve transition to complete + 10. reject transition to ready + 11. reject-feedback requirement + 12. non-review action disablement + +**Files modified:** +- `editor/CMakeLists.txt` — `step436_test` target + +**Verification run:** +- `step436_test` — PASS (12/12) new step coverage +- `step435_test` — PASS (12/12) regression coverage +- `step434_test` — PASS (12/12) regression coverage + +**Architecture gate check:** +- `editor/src/ReviewComparisonView.h` within header-size limit (`118` <= `600`) +- `editor/tests/step436_test.cpp` within test-file size guidance (`173` lines) +- Legacy oversized headers persist: + - `editor/src/ast/Serialization.h` (`1427` > `600`) + - `editor/src/MCPServer.h` (`1940` > `600`) + - `editor/src/HeadlessAgentRPCHandler.h` (`2768` > `600`) + # Roadmap Planning — Sprints 12-25+ ## Status: Planning Complete (Sprints 12-19 detailed, 20-25 in roadmap.md)