diff --git a/plans/005-pass-comments-into-unified-export.md b/plans/005-pass-comments-into-unified-export.md new file mode 100644 index 0000000..9da119c --- /dev/null +++ b/plans/005-pass-comments-into-unified-export.md @@ -0,0 +1,237 @@ +# Plan 005: Pass reviewer comments into unified AI review export + +> **Executor instructions**: Follow this plan step by step. Run every +> verification command and confirm the expected result before moving to the +> next step. If anything in the "STOP conditions" section occurs, stop and +> report β€” do not improvise. When done, update the status row for this plan +> in `plans/README.md` β€” unless a reviewer dispatched you and told you they +> maintain the index. +> +> **Drift check (run first)**: `git diff --stat 37ac800..HEAD -- services/ReviewGenerator.js server.js test/server.test.js` +> If any in-scope file changed since this plan was written, compare the +> "Current state" excerpts against the live code before proceeding; on a +> mismatch, treat it as a STOP condition. + +## Status + +- **Priority**: P1 +- **Effort**: M +- **Risk**: MED +- **Depends on**: none +- **Category**: bug +- **Planned at**: commit `37ac800`, 2026-08-29 + +## Why this matters + +The visual review UI collects file comments and line comments, then `POST /api/export-for-ai` validates them and reports `commentsIncluded` in the JSON response. `ReviewGenerator.generateUnifiedReview` never accepts those fields, so `AI_REVIEW.md` is diffs-only. Split export (`generateSplitReviews` β†’ `generateFileContent`) already embeds comments. Unified export is the default button in `public/index.html` (`exportForAI`). Reviewer notes are the product; they currently stop at the HTTP seam. + +## Current state + +- `services/ReviewGenerator.js` β€” `generateUnifiedReview` writes `AI_REVIEW.md`. Signature at the time of this plan: + +```100:110:services/ReviewGenerator.js + static async generateUnifiedReview({ includedFiles, excludedFiles = [], largeFiles = [] }) { + const timestamp = new Date().toLocaleString(); + let content = `# πŸ” Code Review - ${timestamp}\n\n`; + content += '**Project:** AI Visual Code Review\n'; + content += '**Generated by:** AI Visual Code Review v2.0\n\n'; +``` + +- The per-file loop builds `fileContent` from status + diff only (starts ~line 136). It never reads `comments` or `lineComments`. +- Split path already embeds comments. Copy that shape, do not invent a new markdown dialect. Excerpt: + +```254:269:services/ReviewGenerator.js + // File Comment + if (fileComment) { + content += `## πŸ’­ Review Comment\n\n${fileComment}\n\n`; + } + + // Line Comments + const fileLineComments = Object.entries(lineComments || {}) + .filter(([lineId]) => lineId.includes(file.replace(/[^a-zA-Z0-9]/g, '_'))); + + if (fileLineComments.length > 0) { + content += '## πŸ” Line Comments\n\n'; + fileLineComments.forEach(([lineId, comment]) => { + content += `- **${lineId}:** ${comment}\n`; + }); + content += '\n'; + } +``` + +- `server.js` destructures comments then drops them: + +```556:624:server.js + const { comments = {}, lineComments = {}, excludedFiles = [] } = req.body; + // ... + const result = await ReviewGenerator.generateUnifiedReview({ + includedFiles, + excludedFiles, + // Pass other options if needed by generateUnifiedReview, + // though currently it handles content generation primarily. + }); + + res.json({ + success: true, + file: 'AI_REVIEW.md', + // ... + commentsIncluded: Object.keys(comments).length, +``` + +- `scripts/export-ai-review.js` also calls `generateUnifiedReview` with files only. After this plan it can pass `{}` for comments; CLI has no comment UI. That is fine. +- There is **no** `test/reviewGenerator.test.js`. `test/server.test.js` hits export with supertest but only checks status codes, not markdown body. +- `GitService` is a singleton (`module.exports = new GitService()`). Tests must mock that module **before** requiring `ReviewGenerator`. +- Conventions: CommonJS `require`, Jest. Model new tests after `test/diffService.test.js` (describe/test, no TypeScript). Do not add TypeScript. + +## Commands you will need + +| Purpose | Command | Expected on success | +|---------|---------|---------------------| +| Tests | `npm test` | exit 0 | +| ReviewGenerator tests | `npx jest test/reviewGenerator.test.js --verbose` | exit 0, new cases pass | +| Lint | `npm run lint` | exit 0 (warnings allowed) | +| Prove comments in signature | `rg -n "comments" services/ReviewGenerator.js` | `generateUnifiedReview` destructure includes `comments` | + +## Scope + +**In scope**: + +- `services/ReviewGenerator.js` +- `server.js` (only the `generateUnifiedReview({...})` call in `POST /api/export-for-ai`) +- `test/reviewGenerator.test.js` (create) +- `test/server.test.js` (one assertion that is still HTTP-level and does not require reading `AI_REVIEW.md`) + +**Out of scope**: + +- `src/` wiring (plan 001) +- `public/index.html` markup (plan 006) +- Git listing refactor (plan 007) +- Cache middleware (plan 008) +- `scripts/export-ai-review.js` beyond remaining compatible with extra optional keys +- Changing the HTTP JSON field names +- Writing a real `AI_REVIEW.md` into the repo during tests + +## Git workflow + +- Branch: stay on the current working branch if you were dispatched onto one. Otherwise `prax/unified-export-comments-3d82`. +- Commit message example: `fix(export): include comments in unified AI_REVIEW.md` +- Do not push or open a PR unless the operator instructed it. + +## Steps + +### Step 1: Characterization test that fails on current code + +Create `test/reviewGenerator.test.js`. Mock git and disk **before** requiring the module: + +```js +jest.mock('../services/GitService', () => ({ + getDiffStats: jest.fn().mockResolvedValue('1 file changed'), + getFileStatuses: jest.fn().mockResolvedValue({ 'foo.js': 'M' }), + getDiffForFile: jest.fn().mockResolvedValue('@@ -1,1 +1,1 @@\n-old\n+new\n') +})); + +const fs = require('fs'); +const ReviewGenerator = require('../services/ReviewGenerator'); + +describe('generateUnifiedReview comments', () => { + let writeSpy; + + beforeEach(() => { + writeSpy = jest.spyOn(fs.promises, 'writeFile').mockResolvedValue(); + }); + + afterEach(() => { + writeSpy.mockRestore(); + }); + + test('embeds file comments in the markdown written to disk', async () => { + await ReviewGenerator.generateUnifiedReview({ + includedFiles: ['foo.js'], + excludedFiles: [], + comments: { 'foo.js': 'UNIQUE_REVIEW_NOTE_ABC' }, + lineComments: { 'foo_js_1_1': 'LINE_NOTE_XYZ' } + }); + + expect(writeSpy).toHaveBeenCalled(); + const content = writeSpy.mock.calls[0][1]; + expect(content).toContain('UNIQUE_REVIEW_NOTE_ABC'); + expect(content).toContain('LINE_NOTE_XYZ'); + }); +}); +``` + +On current `main` this test **must fail** (comments ignored). If it passes before you edit `ReviewGenerator.js`, STOP β€” the bug is already gone or the mock is wrong. + +**Verify**: `npx jest test/reviewGenerator.test.js --verbose` β†’ the new test fails because the unique strings are absent. + +### Step 2: Extend `generateUnifiedReview` interface + +In `services/ReviewGenerator.js`: + +1. Destructure `comments = {}` and `lineComments = {}` next to `includedFiles`. +2. Inside the per-file `includedFiles.map` callback, after status/type markdown and **before** the diff block, insert the same comment + line-comment markdown as `generateFileContent` (same headings: `## πŸ’­ Review Comment` and `## πŸ” Line Comments`). Use `comments[file]` and the same `lineId.includes(file.replace(/[^a-zA-Z0-9]/g, '_'))` filter. +3. Do not add a second checklist per file. Keep `getChecklistTemplate()` once at the end as today. + +**Verify**: `npx jest test/reviewGenerator.test.js --verbose` β†’ the characterization test now passes. + +### Step 3: Pass comments from `server.js` + +Change only the `generateUnifiedReview` call in `POST /api/export-for-ai` to: + +```js + const result = await ReviewGenerator.generateUnifiedReview({ + includedFiles, + excludedFiles, + comments, + lineComments + }); +``` + +Leave `POST /api/export-individual-reviews` alone (it already passes comments into `generateSplitReviews`). + +**Verify**: `rg -n "generateUnifiedReview" server.js` β†’ the object includes `comments` and `lineComments`. + +### Step 4: HTTP smoke in existing server tests + +In `test/server.test.js`, in the existing `POST /api/export-for-ai should accept valid request` case, keep status `[200, 400]`. Add a **new** test that invalid comments still 400 (already covered). Add one comment: no new HTTP markdown assertion (file is written to `process.cwd()`). The unit test in step 1 is the regression lock. + +**Verify**: `npx jest test/server.test.js --verbose` β†’ all pass. + +### Step 5: Full suite + +**Verify**: `npm test` β†’ exit 0. Confirm no leftover `AI_REVIEW.md` from tests (`git status` clean of that file). If a test wrote one, delete it and fix the mock. + +## Test plan + +- New file `test/reviewGenerator.test.js`: + - Happy path: file comment + line comment appear in `writeFile` content. + - Empty comments: still writes markdown, no throw (add this second test). + - Pattern: `test/diffService.test.js` (plain Jest, no supertest). +- HTTP: existing export tests still pass. +- Verification: `npm test` β†’ all pass, including the new file. + +## Done criteria + +- [ ] `npm test` exits 0 +- [ ] `npx jest test/reviewGenerator.test.js` exits 0 +- [ ] `rg -n "UNIQUE_REVIEW_NOTE"` is only in the test file, not in `services/` +- [ ] `generateUnifiedReview` destructure includes `comments` and `lineComments` +- [ ] `server.js` passes `comments` and `lineComments` into that call +- [ ] No files outside the in-scope list are modified (`git status`) +- [ ] `plans/README.md` status row for 005 is DONE + +## STOP conditions + +- The code at the locations in "Current state" doesn't match the excerpts. +- Step 1's test passes **before** you change `ReviewGenerator.js`. +- Making the test pass appears to require mocking `child_process` instead of `GitService` (wrong mock β€” fix the mock, don't rewrite Git). +- A step's verification fails twice after a reasonable fix attempt. +- The fix appears to require touching `public/index.html` or `src/`. +- You discover `generateUnifiedReview` already takes and renders comments (bug already fixed). + +## Maintenance notes + +- Any new unified-export field (large-file skips, extra metadata) belongs on this same interface so HTTP and CLI stay aligned. +- Reviewers should confirm comments are interpolated as markdown text, not HTML, matching `generateFileContent`. +- Split export already had comments; do not "simplify" by deleting that path. +- Follow-up deferred: plan 007 (Git listing) and plan 008 (dead cache). diff --git a/plans/006-repair-file-header-markup.md b/plans/006-repair-file-header-markup.md new file mode 100644 index 0000000..0e1d62f --- /dev/null +++ b/plans/006-repair-file-header-markup.md @@ -0,0 +1,174 @@ +# Plan 006: Repair duplicated file-header markup in the review page + +> **Executor instructions**: Follow this plan step by step. Run every +> verification command and confirm the expected result before moving to the +> next step. If anything in the "STOP conditions" section occurs, stop and +> report β€” do not improvise. When done, update the status row for this plan +> in `plans/README.md` β€” unless a reviewer dispatched you and told you they +> maintain the index. +> +> **Drift check (run first)**: `git diff --stat 37ac800..HEAD -- public/index.html test/public-file-header.test.js` +> If any in-scope file changed since this plan was written, compare the +> "Current state" excerpts against the live code before proceeding; on a +> mismatch, treat it as a STOP condition. + +## Status + +- **Priority**: P1 +- **Effort**: S +- **Risk**: MED +- **Depends on**: none +- **Category**: bug +- **Planned at**: commit `37ac800`, 2026-08-29 + +## Why this matters + +`public/index.html` is the highest-churn file in `git log`. `loadFiles()` builds each row with **four** stacked `file-header` nodes (leftover from merged accordion/a11y PRs). Duplicate `id`s on the include checkbox, mixed `onclick="toggleFile('${file}', ...)"` (string-built) and `data-file` handlers, and unclosed `file-path` tags mean `toggleFile`’s `diffDiv.previousElementSibling` is not a reliable header. Keyboard a11y and include-checkboxes do not have a single path. This is a product bug in the default UI, not a style nit. + +## Current state + +- `public/index.html` β€” single-page review UI. ESLint ignores this file (`.eslintrc.js` `ignorePatterns` includes `public/index.html`), so markup bugs will not show up in `npm run lint`. +- Inside `async function loadFiles()`, after `const fileDiv = document.createElement('div');`, `fileDiv.innerHTML` starts approximately at line 1200. The template currently opens **four** `class="file-header"` divs before one `file-diff`. Excerpt of the start of that blob: + +```1199:1220:public/index.html + const fileDiv = document.createElement('div'); + fileDiv.className = 'file-item'; + fileDiv.innerHTML = ` +