docs(plans): add improve plans 005–009 for export, UI, and git listing - #39
Conversation
Reconcile the second standard improve pass at 37ac800. Keep 001–004 TODO. New plans cover unified-export comments, file-header markup, GitService listing, dead GET cache, and CLI listing through the same Git module. Co-authored-by: Prax Lannister <praxstack@users.noreply.github.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ca91a9400
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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. |
There was a problem hiding this comment.
When staged filenames overlap after normalization, this includes filter assigns comments to the wrong file. For example, the UI builds a foo.js.map line ID such as foo_js_map_0_0, which also includes normalized foo.js (foo_js), so the unified export duplicates that note under both files. Carry or compare the exact owning filename and add an overlapping-filename regression test rather than copying the split-export bug.
Useful? React with 👍 / 👎.
|
|
||
| Replace inline listing as follows. Keep `handleAsyncRoute` and existing HTTP status codes. | ||
|
|
||
| 1. **`GET /api/health`**: `const stagedFiles = await GitService.getStagedFiles()` for `stagedCount` (`.length`). Keep unstaged count from porcelain **or** add `getUnstagedModifiedCount` if you can do it in ≤15 lines in `GitService`. Prefer a small `getPorcelainLines()` private-to-file helper rather than a fourth copy of `execute('status-porcelain')` in the route. Minimum bar: `stagedCount` comes from `getStagedFiles().length`. If you still parse porcelain in health for unstaged ` M` / `??`, put that parse in `GitService.getUnstagedChangeCount()` rather than the route. |
There was a problem hiding this comment.
When Git is unavailable, the instructed direct await GitService.getStagedFiles() rejects and handleAsyncRoute turns /api/health into a 500. The current route uses Promise.allSettled and returns 200 with default counts even when either subprocess fails; this is required by the Docker image, which starts the server without installing Git or copying a repository and then probes this endpoint. Explicitly retain independent catches/defaults around the named service calls.
Useful? React with 👍 / 👎.
| - `onclick="toggleFile(this.getAttribute('data-file'), this.getAttribute('data-file-id'))"` | ||
| - `onkeydown="handleFileHeaderKeydown(event, this.getAttribute('data-file'), this.getAttribute('data-file-id'))"` | ||
| - One `div.file-path` containing expand icon `id="icon-${escapeHtml(fileId)}"`, icon text, `escapeHtml(file)`. | ||
| - One include checkbox: **one** `id="select-${escapeHtml(fileId)}"`, `data-filename`, `data-file-id`, `onchange` via `getAttribute` only, `onclick="event.stopPropagation();"` on the label. | ||
| - One comment button: `data-file` / `data-file-id`, `showCommentModal` via `getAttribute`, `aria-label="Add comment for ${escapeHtml(file)}"`. |
There was a problem hiding this comment.
When a keyboard user presses Space or Enter on the include checkbox or comment button, the event bubbles to this header handler; the existing handleFileHeaderKeydown unconditionally calls preventDefault() and toggleFile, suppressing the child control's normal activation and opening the diff instead. Guard for event.target === event.currentTarget, stop keydown propagation on the controls, or move the controls outside the element acting as a button.
Useful? React with 👍 / 👎.
| Add a method that: | ||
|
|
||
| 1. `execute('status-porcelain')` | ||
| 2. Keeps lines whose first two characters include a working-tree `D` in the same way the staged-files route does today: `line.startsWith(' D') || line.startsWith('AD')` |
There was a problem hiding this comment.
When a tracked file is staged as modified and then removed from the worktree, porcelain reports MD path, so neither prescribed prefix matches even though the second status character is D. Local verification showed git status --porcelain=v1 returning MD mod while the current CLI's git ls-files --deleted returned mod; git ls-files -h documents --deleted as “show deleted files in the output.” Plan 009 therefore loses an existing deletion warning after adopting this method. Check the worktree status character and add controlled MD/RD parser cases.
Useful? React with 👍 / 👎.
| - [ ] No files outside the in-scope list are modified (`git status`) | ||
| - [ ] `plans/README.md` status row for 005 is DONE |
There was a problem hiding this comment.
For the default executor, these adjacent completion criteria are mutually exclusive: the plan requires changing the plans/README.md status row, but that file is absent from the in-scope list and all out-of-scope modifications are forbidden. Plans 006–009 repeat the same contradiction, so an executor following the strict stop/do-not-improvise instructions cannot declare any of them complete. Add plans/README.md as an explicit bookkeeping exception or in-scope file in each plan.
Useful? React with 👍 / 👎.
| 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. |
There was a problem hiding this comment.
When getDiffForFile rejects for a non-deleted file because of a Git timeout, command failure, or index race, the existing inner catch rethrows and the outer per-file catch returns a failure result; the final builder then emits only the generic diff error and discards the accumulated fileContent, including the newly inserted reviewer comments. Split export catches the diff failure in place and preserves those notes. Add a rejection-path test and retain the comment markdown when emitting the per-file error.
Useful? React with 👍 / 👎.
2 participants
📋 Description
Advisor-only
/improveplans. No application code in this PR.Reconciles the existing 001–004 index and adds 005–009 for the highest-leverage new findings (unified export dropping comments, stacked
file-headermarkup, Git listing copied in routes, dead cache middleware, CLI gitexecFileSync).The
/improve-codebase-architectureHTML report is not in git. It lives at/tmp/architecture-review-20260829-143802.html(artifact copy/opt/cursor/artifacts/architecture-review.html). Top recommendation: deepen the AI review export module (services/ReviewGenerator.js).🔗 Related Issue
No issue.
🔄 Type of Change
✅ Checklist
📝 Additional Notes
Do not implement from this PR unless you run
/improve execute <plan>. Architecture grilling has not started — pick a candidate first.