Update header Git dropdown review flow#156
Open
friuns2 wants to merge 29 commits into
Open
Conversation
Review Summary by Qodo(Agentic_describe updated until commit 33764be)Update header Git dropdown with commit file review mode and workspace summary
WalkthroughsDescription• **Redesigned header Git dropdown** with three-column layout: branches, commits, and commit file details browser • **Added commit-scoped review mode** enabling file review within specific commits with commitSha parameter support • **Implemented commit file listing** via new /codex-api/git/commit-files endpoint with line counts and status labels • **Added workspace summary endpoint** (/codex-api/review/summary) to retrieve change statistics • **Simplified ReviewPane component** to focus on changes view, removing tab-based UI and findings display • **Enhanced untracked file handling** using git ls-files with null-byte delimiters and preservation during git operations • **Fixed z-index layering** for proper modal behavior with adjusted stacking contexts across layout components • **Extended API types** to support 'commit' review scope and commit-specific review snapshots • **Updated Chinese translations** for commit review UI elements • **Expanded test coverage** from 14 to 30 steps with comprehensive validation for commit file review, worktree counts, and dropdown layering • **Updated agent guidelines** for dev server persistence during active UI work Diagramflowchart LR
A["Git Commit Data"] -->|"buildCommitDiff"| B["Commit File Changes"]
B -->|"/codex-api/git/commit-files"| C["HeaderGitBranchDropdown"]
C -->|"onOpenContentHeaderCommitFile"| D["ReviewPane"]
D -->|"commitSha prop"| E["Commit-Scoped Review"]
A -->|"buildReviewSummary"| F["Workspace Summary"]
F -->|"/codex-api/review/summary"| G["App State"]
G -->|"threadWorktreeChangeSummary"| C
H["DesktopLayout Z-Index"] -->|"isolate + z-index"| D
File Changes1. src/server/reviewGit.ts
|
Code Review by Qodo
1. Untracked filenames get trimmed
|
|
Persistent review updated to latest commit c55825a |
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
|
Persistent review updated to latest commit 597a12b |
Comment on lines
+169
to
+177
| <button | ||
| v-if="branch.value === selectedBranch && branch.value !== currentBranch" | ||
| class="header-git-branch-checkout" | ||
| type="button" | ||
| :disabled="busy" | ||
| @click="emit('checkoutBranch', branch.value)" | ||
| > | ||
| Checkout | ||
| </button> |
There was a problem hiding this comment.
1. Remote checkout detaches head 🐞 Bug ≡ Correctness
HeaderGitBranchDropdown shows a "Checkout" action for remote branches, but the server checkout route accepts remote ref names and runs git checkout <ref>, which can detach HEAD and break the intended branch-based flow.
Agent Prompt
### Issue description
Remote branches (e.g. `origin/main`) can be selected and then checked out via the new `Checkout` button. The server’s `/codex-api/git/checkout` route does not restrict to local branches and calls `git checkout <branch>`, which will commonly result in a detached HEAD for remote refs.
### Issue Context
- UI already surfaces `branch.isRemote` (badge), but the `Checkout` button condition does not account for it.
- Server-side reset-to-commit explicitly enforces local branches via `assertLocalGitBranch`, but checkout does not.
### Fix Focus Areas
- src/components/content/HeaderGitBranchDropdown.vue[155-177]
- src/server/codexAppServerBridge.ts[6552-6584]
### What to change
1. UI: Hide or disable the `Checkout` button for `branch.isRemote === true` (and optionally show a tooltip/message).
2. Server: Add `await assertLocalGitBranch(gitRoot, targetBranch)` in the checkout route (or implement a safe remote checkout behavior like creating a local tracking branch explicitly, if that’s desired).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Persistent review updated to latest commit 374c8c2 |
Comment on lines
785
to
+857
| #### Steps | ||
| 1. In light theme, open the Git dropdown in the thread header. | ||
| 2. Confirm the trigger shows the current branch, or the detached commit subject if the repository is already detached. | ||
| 3. Click `Review` and confirm the review pane opens; click it again and confirm the pane toggles. | ||
| 4. Type part of a branch name in search and confirm the branch list filters. | ||
| 5. Select a different branch with a clean worktree and confirm the header updates to that branch. | ||
| 6. Expand a branch row and confirm recent commits load with short SHA, subject, and date. | ||
| 7. Expand a remote branch row and confirm its commit rows are disabled with a tooltip explaining remote branches cannot be reset. | ||
| 8. Select an older commit on the disposable local branch and confirm the header stays on that branch instead of entering detached HEAD. | ||
| 9. Confirm `git -C <thread-cwd> rev-parse --abbrev-ref HEAD` still prints the branch name and `git -C <thread-cwd> rev-parse --short HEAD` matches the selected commit. | ||
| 10. Reopen/expand the same branch and confirm commits that were ahead of the reset target still appear, with the selected branch HEAD marked `current`. | ||
| 11. Repeat reset on the same branch several times and confirm the dropdown still opens quickly and shows recent reset-history commits. | ||
| 12. Create a tracked uncommitted change, try to switch branch or reset to a commit, and confirm the dropdown shows a dirty-worktree error instead of switching or resetting. | ||
| 13. Create only an untracked file, try to reset to a commit, and confirm the reset proceeds unless Git reports the untracked file would be overwritten. | ||
| 14. Switch to dark theme and repeat steps 1, 2, 4, 6, 7, 10, 12, and 13. | ||
|
|
||
| #### Expected Results | ||
| - The header dropdown exposes Review, current checkout state, searchable branches, and inline commits. | ||
| - Branch switching and branch reset-to-commit are blocked by tracked uncommitted changes, but untracked-only changes are allowed unless Git would overwrite them. | ||
| - Commit selection resets the local branch to that commit instead of detaching HEAD. | ||
| - Remote branch commit rows are inspectable but cannot trigger local branch reset. | ||
| 3. Confirm the menu initially shows only commits on the left and branches on the right, with no commit-files panel before a commit is selected. | ||
| 4. Confirm the left column defaults to the current branch and shows no more than 50 recent commits with short SHA, subject, and date. | ||
| 5. Confirm the open dropdown visually layers above the sidebar and above the Review pane if the pane is already open. | ||
| 6. Confirm the top action is styled as a button, reads `Review Worktree Changes`, and shows `+`/`-` line counts; click it and confirm the dropdown stays open while the review pane opens above it directly to changes without showing a `Findings` tab or `Run review` button; click the Review pane `X` and confirm only the pane closes while the dropdown stays open; click `Review Worktree Changes` again and confirm the pane toggles open. | ||
| 7. Type part of a commit subject or short SHA in the left commit search and confirm the commit list filters. | ||
| 8. Turn off `Reset-history refs` and confirm the commit list reloads without saved reset-history refs. | ||
| 9. Turn `Reset-history refs` back on and confirm saved reset-history commits reappear when available. | ||
| 10. Toggle `Reset-history refs` while the commit list is still loading and confirm the list reloads for the new toggle state instead of staying on the previous result. | ||
| 11. Type part of a branch name in search and confirm the branch list filters. | ||
| 12. Click a different branch row and confirm the left commit list changes to that branch without immediately switching checkout. | ||
| 13. Use the branch row `Checkout` action with a clean worktree and confirm the header updates to that branch. | ||
| 14. Confirm local branches appear first and remote branches appear at the end of the branch list. | ||
| 15. Select an older commit on the disposable local branch and confirm the dropdown widens and shows a left-side file panel with that commit subject, file changes, per-file `+`/`-` line counts, and a `Reset` button without changing HEAD. | ||
| 16. Click a commit ref badge and confirm the full commit SHA is copied to the clipboard without changing the selected commit. | ||
| 17. Click a file in the selected commit details and confirm the dropdown closes and the Review pane opens in commit mode, with the selected commit diff loaded and that file selected without auto-centering the first hunk. | ||
| 18. Click `Reset` for the selected commit and confirm the header stays on that branch instead of entering detached HEAD. | ||
| 19. Confirm `git -C <thread-cwd> rev-parse --abbrev-ref HEAD` still prints the branch name and `git -C <thread-cwd> rev-parse --short HEAD` matches the selected commit. | ||
| 20. Reopen/select the same branch and confirm commits that were ahead of the reset target still appear, with the selected branch HEAD marked `current`. | ||
| 21. Repeat reset on the same branch several times and confirm the dropdown still opens quickly and shows recent reset-history commits. | ||
| 22. Create a tracked uncommitted change, try to switch branch or reset to a commit, and confirm the dropdown shows a dirty-worktree error instead of switching or resetting. | ||
| 23. Create only an untracked file whose path does not exist in the target commit, try to reset to a commit, and confirm the reset proceeds while the untracked file remains in place. | ||
| 24. Create only an untracked file whose path exists in the target commit, try to reset to that target, and confirm the reset proceeds and the untracked file is moved under `.codex/untracked-backups/` instead of being overwritten. | ||
| 25. At a mobile viewport around 375px wide, select a commit and confirm the dropdown fits inside the viewport with branches first, commits second, and selected-commit files last, stacked vertically instead of squeezed into columns. | ||
| 26. Narrow the Review pane file list and confirm changed-file rows do not inherit folder-depth indentation, long names truncate on one line instead of wrapping vertically, and the `+`/`-` counts remain visible. | ||
| 27. At a mobile viewport around 375px wide, open the Review pane, scroll the diff content vertically, and confirm the `X` close button remains visible and tappable in the top-right corner. | ||
| 28. Switch to dark theme and repeat steps 1, 2, 3, 4, 5, 7, 8, 9, 10, 11, 14, 15, 16, 17, 20, 22, 23, 24, 25, 26, and 27. | ||
|
|
||
| #### Preserved Prior Coverage | ||
| 1. Click `Review Worktree Changes` and confirm the review pane opens; click it again and confirm the pane toggles. | ||
| 2. Type part of a branch name in search and confirm the branch list filters. | ||
| 3. Select a different branch with a clean worktree using the checkout action and confirm the header updates to that branch. | ||
| 4. Select a branch row and confirm recent commits load with short SHA, subject, and date. | ||
| 5. Confirm remote branch rows are inspectable, appear after local branches, and do not trigger local reset without a supported local branch action. | ||
| 6. Select an older commit on the disposable local branch and confirm the header stays on that branch instead of entering detached HEAD. | ||
| 7. Confirm `git -C <thread-cwd> rev-parse --abbrev-ref HEAD` still prints the branch name and `git -C <thread-cwd> rev-parse --short HEAD` matches the selected commit after reset. | ||
| 8. Reopen/select the same branch and confirm commits that were ahead of the reset target still appear, with the selected branch HEAD marked `current`. | ||
| 9. Repeat reset on the same branch several times and confirm the dropdown still opens quickly and shows recent reset-history commits. | ||
| 10. Create a tracked uncommitted change, try to switch branch or reset to a commit, and confirm the dropdown shows a dirty-worktree error instead of switching or resetting. | ||
| 11. Create only an untracked file, try to reset to a commit, and confirm the reset proceeds while preserving the untracked file in place or moving it to `.codex/untracked-backups/` if the target would overwrite it. | ||
| 12. Switch to dark theme and repeat the branch filtering, commit loading, reset-history, dirty-worktree, and untracked-file checks. | ||
|
|
||
| #### Expected Results | ||
| - The header dropdown exposes Review, current checkout state, a left-side commit list, and a right-side searchable branch list before a commit is selected. | ||
| - The selected-commit file panel is hidden until commit selection, then appears on the left and expands the dropdown width. | ||
| - Each selected-commit file row shows added and removed line counts, using `-` for binary or unavailable counts. | ||
| - The dropdown layer is viewport-positioned and appears above the sidebar when open. | ||
| - The Review pane renders above the open dropdown and app chrome. | ||
| - Clicking the dropdown `Review Worktree Changes` button keeps the dropdown open while toggling the Review pane. | ||
| - Clicking the Review pane `X` while the dropdown is open closes only the Review pane and leaves the dropdown open. | ||
| - The `Review Worktree Changes` button shows current worktree added and removed line counts. | ||
| - The Review pane toolbar keeps `Refresh` but does not show a `Findings` tab or `Run review` button. | ||
| - The current branch commit list loads by default and is capped at 50 commits. | ||
| - The commit list can be searched by SHA, subject, or date without changing the selected branch. | ||
| - Reset-history refs can be shown or hidden from the commit list without changing the selected branch. | ||
| - Reset-history toggles are keyed by both branch and reset-history state, so an in-flight load for one state does not suppress loading the other state. | ||
| - Branch switching and branch reset-to-commit are blocked by tracked uncommitted changes, but untracked-only changes are preserved and allowed. | ||
| - Commit selection opens file details without resetting or detaching HEAD. | ||
| - Commit ref badges copy the full SHA to the clipboard without triggering commit selection. | ||
| - The selected commit `Reset` button resets the local branch to that commit instead of detaching HEAD. | ||
| - Clicking a selected commit file opens the Review pane against that commit diff and selects that path without auto-centering the selected hunk. | ||
| - Changed-file rows in the Review pane file list do not inherit folder-depth indentation, so long names have enough room to truncate horizontally instead of wrapping one character per line when the list is narrow. | ||
| - Remote branches appear after local branches in the branch list. | ||
| - Prior branch-search, inline-commit, remote-branch inspection, reset-to-commit, dirty-worktree, and untracked-file manual coverage remains represented in the section. | ||
| - The branch commit list still shows commits that were ahead of the reset target by reading saved internal reset-history refs. | ||
| - Reset-history refs are bounded so repeated resets do not grow commit-list inputs without limit. | ||
| - The selected branch HEAD commit is marked `current` in expanded commit lists. | ||
| - Untracked files that would collide with target tracked files are moved to `.codex/untracked-backups/` before checkout/reset. | ||
| - The selected branch HEAD commit is marked `current` in the commit list. | ||
| - The mobile Review pane keeps its close button visible above the app chrome in both light theme and dark theme. | ||
| - The mobile Review pane diff area scrolls vertically without moving or hiding the pane header. | ||
| - On mobile, branches, commits, and selected-commit file details stack vertically in that order and stay inside the viewport in both light theme and dark theme. |
There was a problem hiding this comment.
1. tests.md missing cleanup notes 📘 Rule violation ⚙ Maintainability
The updated manual test section introduces steps that change Git state and create untracked/tracked files, but it does not include any rollback/cleanup guidance. This makes the instructions non-reproducible and can leave the repo/worktree (and .codex/untracked-backups/) in a modified state after testing.
Agent Prompt
## Issue description
The updated `tests.md` section for the header Git dropdown/review flow lacks rollback/cleanup notes, despite instructing testers to reset branches and create untracked/tracked files.
## Issue Context
Per the manual test documentation requirements, sections should include rollback/cleanup notes when applicable. This feature’s test steps include operations that can alter checkout state and move files into `.codex/untracked-backups/`, so cleanup guidance is applicable.
## Fix Focus Areas
- tests.md[785-857]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+434
to
+441
| async function buildCommitDiff(repoRoot: string, commitSha: string): Promise<{ diffText: string; commitSha: string }> { | ||
| const resolvedSha = await runCommandCapture('git', ['rev-parse', '--verify', `${commitSha}^{commit}`], { cwd: repoRoot }) | ||
| const diffText = await runCommandCapture( | ||
| 'git', | ||
| ['diff-tree', '--root', '--no-commit-id', '--no-ext-diff', '--find-renames', '--patch', resolvedSha], | ||
| { cwd: repoRoot }, | ||
| ) | ||
| return { diffText, commitSha: resolvedSha } |
There was a problem hiding this comment.
2. Commit diff not recursive 🐞 Bug ≡ Correctness
buildCommitDiff() runs git diff-tree without -r, so commit review mode can return an empty/incomplete patch for commits whose changes are only in subdirectories. This breaks commit-file review mode because the Review pane snapshot may show no files/hunks despite real changes.
Agent Prompt
### Issue description
Commit review snapshots are generated via `git diff-tree` without the `-r` (recursive) flag, which can omit file-level diffs for commits that only modify files inside subdirectories.
### Issue Context
The server already uses `git diff-tree ... -r` elsewhere (commit-files endpoint), indicating recursion is expected for correct commit traversal.
### Fix Focus Areas
- src/server/reviewGit.ts[434-441]
### Implementation notes
- Update the `runCommandCapture('git', ['diff-tree', ...])` argv in `buildCommitDiff` to include `-r` (and keep existing flags like `--root`, `--find-renames`, `--patch`).
- Consider matching the commit-files endpoint behavior/flags for consistency.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Persistent review updated to latest commit 88653f3 |
Comment on lines
+933
to
+937
| @apply fixed inset-3 z-[1200] flex min-h-0 min-w-0 flex-col overflow-hidden rounded-2xl border border-zinc-200 bg-white shadow-2xl; | ||
| } | ||
|
|
||
| .review-pane.is-mobile { | ||
| @apply fixed inset-0 z-40 rounded-none border-0; | ||
| @apply inset-0 rounded-none border-0; |
There was a problem hiding this comment.
1. review-pane lacks dark styles 📘 Rule violation ≡ Correctness
The updated Review pane overlay uses light-surface styling (e.g., bg-white) but no dark-theme override is added in src/style.css, so it will render as a light surface in dark mode. This violates the requirement to centralize decisive dark overrides for shared route surfaces in the global stylesheet.
Agent Prompt
## Issue description
The Review pane is a shared, full-surface overlay and now explicitly uses light-surface styles (`bg-white`, light borders). There are no corresponding global `:root.dark ...` overrides in `src/style.css`, risking a light UI surface in dark mode.
## Issue Context
Compliance requires decisive dark-theme overrides for shared route surfaces/large feature UIs to live in `src/style.css`.
## Fix Focus Areas
- src/components/content/ReviewPane.vue[933-937]
- src/style.css[891-992]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Persistent review updated to latest commit 9c42465 |
Comment on lines
+371
to
+374
| const statusOutput = await runCommandCapture('git', ['status', '--porcelain=v1', '--untracked-files=all'], { cwd: repoRoot }) | ||
| for (const path of parseUntrackedPaths(statusOutput)) { | ||
| summary = addReviewSummary(summary, await summarizeUntrackedFile(repoRoot, path)) | ||
| } |
There was a problem hiding this comment.
1. Untracked filenames get trimmed 🐞 Bug ≡ Correctness
The new /codex-api/review/summary path can return wrong counts (or 500) because untracked paths are parsed from git status --porcelain=v1 with .trim(), which changes filenames and can’t represent Git’s quoted/escaped porcelain paths. That trimmed path is then fed into stat()/readFile(), so repos with untracked filenames that include leading/trailing whitespace (or require quoting) can break the header worktree +/- summary.
Agent Prompt
## Issue description
The review summary endpoint computes untracked file counts using `git status --porcelain=v1` and `parseUntrackedPaths()` which trims path text. This can corrupt filenames (leading/trailing whitespace) and fails for porcelain-quoted paths, leading to incorrect summary counts or thrown fs errors.
## Issue Context
- `/codex-api/review/summary` is now used to power the header worktree +/- counts.
- `parseUntrackedPaths()` currently does `line.slice(3).trim()`.
- `summarizeUntrackedFile()` calls `stat()`/`readFile()` using that derived path.
## Fix Focus Areas
- src/server/reviewGit.ts[281-289]
- src/server/reviewGit.ts[335-346]
- src/server/reviewGit.ts[348-375]
- src/server/reviewGit.ts[902-924]
## Implementation notes
- Prefer a NUL-delimited source of untracked paths (e.g. `git ls-files --others --exclude-standard -z`) to avoid porcelain quoting/escaping entirely.
- Do not apply `.trim()` to filenames.
- Make `summarizeUntrackedFile()` robust: catch ENOENT / non-file cases and return a zero summary instead of throwing.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+7037
to
+7055
| for (const line of numstatOutput.split('\n')) { | ||
| const parts = line.split('\t') | ||
| const addedRaw = parts[0]?.trim() ?? '' | ||
| const removedRaw = parts[1]?.trim() ?? '' | ||
| const path = (parts.length >= 4 ? parts[3] : parts[2])?.trim() ?? '' | ||
| if (!path) continue | ||
| const addedLineCount = /^\d+$/.test(addedRaw) ? Number(addedRaw) : null | ||
| const removedLineCount = /^\d+$/.test(removedRaw) ? Number(removedRaw) : null | ||
| lineCountsByPath.set(path, { addedLineCount, removedLineCount }) | ||
| } | ||
| const files = output.split('\n').flatMap((line) => { | ||
| const parts = line.split('\t').map((part) => part.trim()).filter(Boolean) | ||
| const status = parts[0] ?? '' | ||
| if (!status) return [] | ||
| const statusKind = status.charAt(0) | ||
| const isRenameOrCopy = (statusKind === 'R' || statusKind === 'C') && parts.length >= 3 | ||
| const path = isRenameOrCopy ? parts[2] : parts[1] | ||
| const previousPath = isRenameOrCopy ? parts[1] : null | ||
| if (!path) return [] |
There was a problem hiding this comment.
2. Commit file paths altered 🐞 Bug ≡ Correctness
The new /codex-api/git/commit-files endpoint (and the client normalizer) trims and tab-splits paths coming from git diff-tree, which can corrupt legitimate filenames containing leading/trailing whitespace and can desync name-status vs numstat path matching. This can cause the commit file list to show wrong/missing entries and prevent opening the intended file in commit review mode.
Agent Prompt
## Issue description
The commit-files API parses `git diff-tree` output using newline/tab splitting and applies `.trim()` to parsed path fields. This can change the actual filename (e.g. leading/trailing spaces) and makes parsing ambiguous for filenames containing special characters. The client then trims again, further ensuring the UI cannot round-trip exact paths.
## Issue Context
- Server: `/codex-api/git/commit-files` builds `files` via `diff-tree --name-status` and separately parses `diff-tree --numstat`, trimming the `path` field in both.
- Client: `getGitCommitFiles()` trims `record.path` and `record.previousPath`.
## Fix Focus Areas
- src/server/codexAppServerBridge.ts[7011-7071]
- src/api/codexGateway.ts[2725-2751]
## Implementation notes
- Switch both `--name-status` and `--numstat` invocations to `-z` and parse NUL-delimited records.
- Do not `.trim()` path fields; treat them as opaque.
- Ensure rename/copy parsing keeps both previousPath and path exactly as emitted.
- Update client normalizer to avoid trimming `path`/`previousPath` (it may still validate type/emptiness without modifying content).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Persistent review updated to latest commit 7c8313e |
|
Persistent review updated to latest commit 9cad38f |
|
Persistent review updated to latest commit a9105d6 |
|
Persistent review updated to latest commit 33764be |
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
2 participants
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.
Updates the header Git dropdown review flow after PR #121, including commit file review mode, worktree change counts, and Review pane layering.