1e374abcd6
13 files reviewed with mission-control design lens. Key additions: - theme: extended indigo scale, 4-level surface hierarchy, 22 terminal tokens, transition/z-index/focus-visible token categories - All screens: keyboard shortcuts, loading/error/empty states hardened - 5 new shared components: StatusDot, SkeletonLoader, Toast, Badge, KeyboardShortcutHint - settings: expanded from 2 to 5 sub-pages (accounts, workspace, danger zone) - review-tab: 3-pane layout, inline comments, file nav, hunk controls - execution-tab: zoom, partial failure state, stale agent detection - dialogs: 2 bugs found (mutation locking, error placement) Total: 4,039 → 9,302 lines (+130% from review pass)
570 lines
28 KiB
Markdown
570 lines
28 KiB
Markdown
# Review Tab (v2)
|
|
|
|
### Route: `/initiatives/$id` (Review tab)
|
|
### Source: `packages/web/src/components/review/ReviewTab.tsx`
|
|
|
|
---
|
|
|
|
## v1 -> v2 Changes
|
|
|
|
| Aspect | v1 | v2 |
|
|
|--------|----|----|
|
|
| Loading state | Plain "Loading diff..." text | Centered `[spinner]` + "Loading diff..." text |
|
|
| Error state | None | `[AlertCircle]` + error message + `[Retry]` button |
|
|
| Unresolved threads | Count in diff header only | Count in header + `[ChevronUp]`/`[ChevronDown]` prev/next-unresolved buttons in sidebar |
|
|
| Empty state | "No changes in this phase" | `[check-circle]` icon + "No changes to review" + description |
|
|
|
|
---
|
|
|
|
## Default State (with diff and threads)
|
|
|
|
```
|
|
+=============================================================================+
|
|
| Files (3) | Diff: src/auth/oauth.ts [Unified|Split] 3 unresolved |=| Threads [^] [v] |
|
|
| ---------------+-------------------------------------------------------------+-+ |
|
|
| * oauth.ts | @@ -12,6 +12,14 @@ |‖| Thread #1 |
|
|
| pkce.ts | import { generatePKCE } from './pkce'; |‖| Line 14 |
|
|
| index.ts | +export async function authorize( [+ comment] |‖| "Should we |
|
|
| | + client: OAuthClient, |‖| validate |
|
|
| | + scopes: string[] |‖| scopes?" |
|
|
| | +) { |‖| [Reply___________] |
|
|
| | + const { verifier, challenge } = generatePKCE(); |‖| [Resolve] |
|
|
| | + // ... |‖| |
|
|
| | +} |‖| Thread #2 |
|
|
| | |‖| Line 28 |
|
|
| | export function validateToken( |‖| "Add expiry |
|
|
| | - token: string |‖| check here" |
|
|
| | + token: string, |‖| [RESOLVED] |
|
|
| | + options?: ValidateOptions |‖| |
|
|
| | |‖| |
|
|
+=============================================================================+
|
|
^
|
|
resize handle (drag)
|
|
```
|
|
|
|
### Layout: three-pane
|
|
|
|
| Pane | Width | Notes |
|
|
|------|-------|-------|
|
|
| File tree | `w-48` (192px), collapsible | Lists all files in the diff. `*` marks files with unresolved threads. Click to navigate. |
|
|
| Diff viewer | flex-1 (fills remaining) | Unified or split view. Scrolls independently. |
|
|
| Threads sidebar | min `240px`, default `320px`, max `480px`, **resizable** via drag handle | `‖` is a 4px drag handle (`cursor-col-resize`). User can drag to resize. Stores width in `localStorage` key `cw-review-sidebar-width`. |
|
|
|
|
### Navigation buttons
|
|
|
|
`[^]` (ChevronUp) = "Previous unresolved". `[v]` (ChevronDown) = "Next unresolved".
|
|
Both scroll the diff viewer to the corresponding unresolved thread and highlight it.
|
|
Disabled when all threads are resolved. Wraps around (last -> first, first -> last).
|
|
Both have visible text labels on wider viewports: `[^ Prev] [Next v]`.
|
|
On narrow viewports (< 1280px), icon-only with tooltips: "Previous unresolved (Shift+N)" / "Next unresolved (N)".
|
|
|
|
### Diff header anatomy
|
|
|
|
```
|
|
+------------------------------------------------------------------------+
|
|
| Diff: <file-path> [Unified | Split] N unresolved |
|
|
+------------------------------------------------------------------------+
|
|
```
|
|
|
|
- File path is the currently-focused file (bold `font-mono text-sm font-medium`)
|
|
- `[Unified | Split]` — segmented toggle, same style as `<ThemeToggle>`. Default: Unified. Persists in `localStorage` key `cw-review-view-mode`.
|
|
- **Unified**: standard diff with `+`/`-` lines interleaved (default)
|
|
- **Split**: side-by-side two-column view — old on left, new on right, synced scroll
|
|
- "N unresolved" uses `text-status-warning-fg` when N > 0, `text-muted-foreground` when 0
|
|
|
|
### Sidebar header anatomy
|
|
|
|
```
|
|
+----------------------------------+
|
|
| Threads (3) [^] [v] |
|
|
+----------------------------------+
|
|
```
|
|
|
|
- `(3)` = total thread count for current file, `text-muted-foreground`
|
|
- `[^]` (ChevronUp icon) scrolls to previous unresolved thread. Tooltip: "Previous unresolved (Shift+N)"
|
|
- `[v]` (ChevronDown icon) scrolls to next unresolved thread. Tooltip: "Next unresolved (N)"
|
|
- Both buttons: `variant="ghost" size="icon-sm"`, disabled state when 0 unresolved
|
|
|
|
---
|
|
|
|
## Loading State
|
|
|
|
```
|
|
+=============================================================================+
|
|
| |
|
|
| |
|
|
| [spinner] |
|
|
| Loading diff... |
|
|
| |
|
|
| |
|
|
+=============================================================================+
|
|
```
|
|
|
|
Centered vertically and horizontally within the tab content area.
|
|
Spinner uses `animate-spin` on a `Loader2` Lucide icon.
|
|
Text is `text-sm text-muted-foreground`.
|
|
|
|
---
|
|
|
|
## Error State
|
|
|
|
```
|
|
+=============================================================================+
|
|
| |
|
|
| |
|
|
| [AlertCircle] |
|
|
| Failed to load diff data |
|
|
| |
|
|
| [ Retry ] |
|
|
| |
|
|
+=============================================================================+
|
|
```
|
|
|
|
- `[AlertCircle]` icon: `h-8 w-8 text-destructive`
|
|
- Error message: `text-sm text-destructive`
|
|
- `[ Retry ]` button: `variant="outline" size="sm"`, calls `diffQuery.refetch()`
|
|
|
|
---
|
|
|
|
## Empty State (no proposals/diffs)
|
|
|
|
```
|
|
+=============================================================================+
|
|
| |
|
|
| [check-circle] |
|
|
| No changes to review |
|
|
| All proposals have been reviewed. |
|
|
| |
|
|
+=============================================================================+
|
|
```
|
|
|
|
- `[check-circle]` icon: `h-8 w-8 text-status-success-fg`
|
|
- Title: `text-sm font-medium`
|
|
- Description: `text-sm text-muted-foreground`
|
|
|
|
This replaces the v1 "No phases pending review" plain text.
|
|
|
|
---
|
|
|
|
## Full Page Context (within initiative detail)
|
|
|
|
```
|
|
+=============================================================================+
|
|
| [CW] [*Initiatives*] Agents *3 Inbox (2) Settings [cmd-k] [sun] * |
|
|
+=============================================================================+
|
|
| < Back |
|
|
| Auth System Overhaul [ACTIVE] [REVIEW] [git] cw/auth [P] backend |
|
|
| [ Content ] [ Plan ] [ Execution ] [*Review*] |
|
|
+-----------------------------------------------------------------------------|
|
|
| |
|
|
| Phase: [*Phase 2: OAuth*] [Phase 3: UI] |
|
|
| |
|
|
| +-----------------------------------------------------------+ |
|
|
| | [play] Start Preview | |
|
|
| +-----------------------------------------------------------+ |
|
|
| |
|
|
| Files (3) | Diff: src/auth/oauth.ts [Uni|Spl] 3 unresolved | Threads [^][v] |
|
|
| ----------+---------------------------------------------------+-+ |
|
|
| * oauth | @@ -12,6 +12,14 @@ |‖| Thread #1 |
|
|
| pkce | import { generatePKCE } from './pkce'; |‖| Line 14 |
|
|
| index | +export async function authorize( |‖| "Should we |
|
|
| | + client: OAuthClient, |‖| validate |
|
|
| | + scopes: string[] |‖| scopes?" |
|
|
| | +) { |‖| [Reply___] |
|
|
| | + const { verifier, challenge } = ... |‖| [Resolve] |
|
|
| | + // ... |‖| |
|
|
| | +} |‖| Thread #2 |
|
|
| | |‖| Line 28 |
|
|
| | export function validateToken( |‖| "Add expiry |
|
|
| | - token: string |‖| check here" |
|
|
| | + token: string, |‖| [RESOLVED] |
|
|
| | + options?: ValidateOptions |‖| |
|
|
| | |‖| |
|
|
+=============================================================================+
|
|
```
|
|
|
|
---
|
|
|
|
## Thread States
|
|
|
|
### Unresolved thread (in sidebar)
|
|
|
|
```
|
|
+----------------------------------+
|
|
| Thread #1 [x] |
|
|
| Line 14 · agent: blue-fox-7 |
|
|
| "Should we validate |
|
|
| scopes before passing |
|
|
| to the provider?" |
|
|
| |
|
|
| [Reply_____________________] |
|
|
| [Resolve] |
|
|
+----------------------------------+
|
|
```
|
|
|
|
- `[x]` = delete thread (destructive, Shift+click to skip confirmation per UI Patterns)
|
|
- Line number is a clickable link — scrolls diff viewer to that line and highlights it
|
|
- Agent attribution shows who created the thread (`text-xs text-muted-foreground font-mono`)
|
|
- Reply input: `<Textarea>` with `rows={1}`, auto-expands. Submit with `Cmd+Enter`.
|
|
- `[Resolve]` button: `variant="outline" size="sm"`
|
|
|
|
### Resolution transition (animation)
|
|
|
|
When `[Resolve]` is clicked:
|
|
|
|
1. **Immediate**: `[Resolve]` button replaced with `[RESOLVED]` badge (no delay)
|
|
2. **300ms**: Thread card fades to `opacity-50` via `transition-opacity duration-300`
|
|
3. **400ms**: Thread collapses to single-line summary via `transition-all duration-400`:
|
|
|
|
```
|
|
+----------------------------------+
|
|
| [v] Thread #2 · Line 28 |
|
|
| [RESOLVED] |
|
|
+----------------------------------+
|
|
```
|
|
|
|
4. Collapsed resolved threads can be expanded by clicking `[v]` (ChevronRight) to reveal full content
|
|
5. The corresponding diff gutter marker transitions from `bg-status-warning-dot` to `bg-status-success-dot`
|
|
6. Unresolved count in header decrements immediately (optimistic update)
|
|
|
|
### Resolved thread — collapsed (default)
|
|
|
|
```
|
|
+----------------------------------+
|
|
| [>] Thread #2 · Line 28 |
|
|
| [RESOLVED] |
|
|
+----------------------------------+
|
|
```
|
|
|
|
### Resolved thread — expanded
|
|
|
|
```
|
|
+----------------------------------+
|
|
| [v] Thread #2 · Line 28 |
|
|
| "Add expiry check here" |
|
|
| [RESOLVED] [Unresolve] |
|
|
+----------------------------------+
|
|
```
|
|
|
|
- Resolved threads: `opacity-50`, collapsed by default
|
|
- `[RESOLVED]` badge uses `bg-status-success-bg text-status-success-fg`
|
|
- `[Unresolve]` text button: `text-xs text-muted-foreground hover:text-foreground` — reverts to unresolved state
|
|
- `[>]`/`[v]` toggle: `ChevronRight`/`ChevronDown`, `variant="ghost" size="icon-xs"`
|
|
|
|
---
|
|
|
|
## Inline Comment Creation
|
|
|
|
How a user starts a new review thread on a diff line.
|
|
|
|
### Trigger: click on gutter line number
|
|
|
|
Every diff line has a clickable gutter area (line number column). Hovering a line shows a `[+]` icon in the gutter (`opacity-0 group-hover:opacity-100`, `text-primary`). Clicking it opens an inline comment form directly below that diff line.
|
|
|
|
```
|
|
| 14 | +export async function authorize(
|
|
| | +---------------------------------------------+
|
|
| | | [avatar] Add a comment... |
|
|
| | | [_____________________________________] |
|
|
| | | [Cancel] [Submit] |
|
|
| | +---------------------------------------------+
|
|
| 15 | + client: OAuthClient,
|
|
```
|
|
|
|
### Trigger: select text in diff, then click "Comment"
|
|
|
|
Selecting text within a diff line reveals a floating toolbar above the selection:
|
|
|
|
```
|
|
| 14 | +export async function ████████████(
|
|
+----------+
|
|
| [Comment]|
|
|
+----------+
|
|
```
|
|
|
|
Clicking `[Comment]` opens the inline form (same as above) with the selected text quoted in the input as a blockquote prefix.
|
|
|
|
### Inline comment form anatomy
|
|
|
|
```
|
|
+----------------------------------------------+
|
|
| [textarea: placeholder "Add a comment..."] |
|
|
| |
|
|
| [Cancel] [Comment] |
|
|
+----------------------------------------------+
|
|
```
|
|
|
|
- Textarea: `<Textarea rows={2}>`, auto-expands, max 6 rows. `font-mono text-sm`.
|
|
- `[Cancel]`: `variant="ghost" size="sm"` — closes form, discards input
|
|
- `[Comment]`: `variant="default" size="sm"` — creates thread, scrolls sidebar to show it
|
|
- `Cmd+Enter` submits, `Escape` cancels
|
|
- On submit: new thread appears in sidebar, gutter line gets a thread marker dot (`bg-status-warning-dot`)
|
|
|
|
### Gutter thread markers
|
|
|
|
Lines with existing threads show a colored dot in the gutter:
|
|
|
|
```
|
|
| 12 | import { generatePKCE } from './pkce';
|
|
| •14 | +export async function authorize( <- dot = thread on this line
|
|
| 15 | + client: OAuthClient,
|
|
```
|
|
|
|
- Unresolved thread dot: `bg-status-warning-dot` (amber)
|
|
- Resolved thread dot: `bg-status-success-dot` (green)
|
|
- Clicking the dot scrolls the sidebar to that thread and highlights it
|
|
|
|
---
|
|
|
|
## File Navigation (File Tree Pane)
|
|
|
|
Left pane listing all files changed in the current phase diff.
|
|
|
|
### File tree anatomy
|
|
|
|
```
|
|
+-------------------+
|
|
| Files (3) |
|
|
| ───────────── |
|
|
| * oauth.ts +5-2 | <- selected (bold), has unresolved threads (*)
|
|
| pkce.ts +28 | <- additions only
|
|
| index.ts +1-1 |
|
|
+-------------------+
|
|
```
|
|
|
|
- Pane width: `w-48` (192px), collapsible via `[PanelLeftClose]` icon in pane header
|
|
- Collapsed state stores in `localStorage` key `cw-review-files-collapsed`
|
|
- `*` prefix on files with unresolved threads (uses `text-status-warning-fg`)
|
|
- `+N-M` shows addition/deletion line counts (`text-diff-add-fg` / `text-diff-remove-fg`)
|
|
- Selected file: `bg-accent font-medium`
|
|
- File paths truncated with ellipsis (`truncate`) — full path in tooltip
|
|
- Sort order: files with unresolved threads first, then alphabetical
|
|
- Keyboard: `j`/`k` to move selection, `Enter` to open file in diff viewer
|
|
|
|
### When file tree is collapsed
|
|
|
|
```
|
|
+--+
|
|
|[>| <- PanelLeftOpen icon, click to expand
|
|
+--+
|
|
```
|
|
|
|
Diff header still shows the current file path so context is never lost.
|
|
|
|
---
|
|
|
|
## Diff View Modes
|
|
|
|
### Unified view (default)
|
|
|
|
Standard interleaved diff — additions and deletions inline, single column. Same as shown in Default State wireframe above.
|
|
|
|
### Split view
|
|
|
|
Side-by-side two-column layout — old file on left, new file on right. Scroll is synced.
|
|
|
|
```
|
|
+------------------------------------------------------------------+
|
|
| Diff: src/auth/oauth.ts [Unified | *Split*] 3 unresolved |
|
|
+------------------------------------------------------------------+
|
|
| OLD (left) | NEW (right) |
|
|
| 12 import { generatePKCE } | 12 import { generatePKCE } |
|
|
| 13 // ... | 13 // ... |
|
|
| 14 ░░░░░░░░░░░░░░░░░░░░░░░░░ | 14 +export async function |
|
|
| ░░░░░░░░░ (empty) | 15 + client: OAuthClient, |
|
|
| ░░░░░░░░░ | 16 + scopes: string[] |
|
|
| ░░░░░░░░░ | 17 +) { |
|
|
| | |
|
|
| 28 - token: string | 31 + token: string, |
|
|
| | 32 + options?: ValidateOpt |
|
|
+------------------------------------------------------------------+
|
|
```
|
|
|
|
- Empty lines on the shorter side filled with `bg-muted/30` hatching
|
|
- Removed lines: `bg-diff-remove-bg` on left only
|
|
- Added lines: `bg-diff-add-bg` on right only
|
|
- Each side has its own line numbers
|
|
- Gutter comment markers (`[+]` on hover, thread dots) appear on both sides
|
|
|
|
---
|
|
|
|
## Hunk-Level Controls
|
|
|
|
Each hunk (`@@ ... @@` header) has accept/reject controls for bulk-accepting or rejecting changes within that hunk.
|
|
|
|
### Hunk header anatomy
|
|
|
|
```
|
|
+------------------------------------------------------------------------+
|
|
| @@ -12,6 +12,14 @@ function setup() [Accept Hunk] [Reject Hunk] |
|
|
+------------------------------------------------------------------------+
|
|
```
|
|
|
|
- `[Accept Hunk]`: `variant="ghost" size="xs"`, `CheckCheck` icon + "Accept". Marks all changes in this hunk as accepted — lines get `bg-diff-add-bg/40` (dimmed, signaling "reviewed").
|
|
- `[Reject Hunk]`: `variant="ghost" size="xs"`, `X` icon + "Reject". Opens a confirmation: "Reject this hunk? This will create a revert suggestion." On confirm, creates a thread with a revert suggestion.
|
|
- Both buttons: `opacity-0 group-hover:opacity-100` (visible on hunk header hover)
|
|
- Accepted hunks show a subtle `[check]` icon in the hunk header
|
|
- Hunk header background: `bg-diff-hunk-bg` (from theme.md diff tokens)
|
|
|
|
---
|
|
|
|
## Partial Diff State (agent still working)
|
|
|
|
When an agent is actively running and producing changes, the diff may be incomplete. This state shows what is available so far with a clear indicator that more changes are expected.
|
|
|
|
```
|
|
+=============================================================================+
|
|
| Files (1) | Diff: src/auth/oauth.ts [Uni|Spl] 0 unresolved | ... |
|
|
| -----------+-----------------------------------------------------+ |
|
|
| ~ oauth.ts | @@ -12,6 +12,8 @@ | |
|
|
| | +export async function authorize( | |
|
|
| | + client: OAuthClient, | |
|
|
| | | |
|
|
| +-----------------------------------------------------+ |
|
|
| | [spinner] Agent blue-fox-7 is still working... | |
|
|
| | Changes may be incomplete. Auto-refreshes on save. | |
|
|
+=============================================================================+
|
|
```
|
|
|
|
- `~` prefix on files being actively modified (uses `text-status-active-fg`)
|
|
- Bottom banner: `bg-status-active-bg border-t border-status-active-border`
|
|
- `[spinner]` is `Loader2 animate-spin` inline with text
|
|
- Diff auto-refreshes when new output detected (via `onAgentOutput` subscription)
|
|
- Thread creation is allowed on partial diffs — threads persist even as diff updates
|
|
- Banner disappears when agent completes (status transitions away from `running`)
|
|
|
|
---
|
|
|
|
## Syntax Highlighting
|
|
|
|
The diff viewer uses syntax-highlighted code rendering. Colors are derived from the **terminal tokens** defined in `theme.md` section 3, ensuring consistent code coloring across the agent output viewer and the diff viewer.
|
|
|
|
| Code element | Token | Example |
|
|
|-------------|-------|---------|
|
|
| Keywords (`export`, `async`, `function`, `const`) | `--terminal-tool` (blue) | `text-terminal-tool` |
|
|
| Strings | `--terminal-result` (green) | `text-terminal-result` |
|
|
| Comments | `--terminal-muted` (dimmed) | `text-terminal-muted` |
|
|
| Types / annotations | `--terminal-system` (grey) | `text-terminal-system` |
|
|
| Errors / invalids | `--terminal-error` (red) | `text-terminal-error` |
|
|
| Default / identifiers | `--foreground` | `text-foreground` |
|
|
|
|
Implementation note: use a lightweight syntax highlighter (e.g., `shiki` with the token mapping above, or a custom Prism theme wired to CSS custom properties). The diff background colors (`--diff-add-bg`, `--diff-remove-bg`) must remain visible through the syntax highlighting — syntax colors are applied to `color` only, never `background-color`.
|
|
|
|
---
|
|
|
|
## Keyboard Shortcuts
|
|
|
|
Review-tab-specific shortcuts, active when the Review tab is focused.
|
|
|
|
| Key | Action | Notes |
|
|
|-----|--------|-------|
|
|
| `n` | Next unresolved thread | Same as `[v]` button. Wraps around. |
|
|
| `Shift+N` | Previous unresolved thread | Same as `[^]` button. Wraps around. |
|
|
| `j` / `k` | Next / previous file in file tree | Moves file selection, loads diff |
|
|
| `]` / `[` | Next / previous hunk | Scrolls diff to next/prev `@@` header |
|
|
| `r` | Resolve focused thread | Only when a thread is focused in sidebar |
|
|
| `c` | Open comment on current line | Opens inline comment form at cursor line |
|
|
| `Escape` | Close inline comment / deselect | Context-dependent |
|
|
| `Cmd+Enter` | Submit comment / reply | When comment textarea is focused |
|
|
| `u` | Toggle unified/split view | Same as `[Unified|Split]` toggle |
|
|
| `f` | Toggle file tree pane | Collapse/expand the file tree |
|
|
| `?` | Show keyboard shortcut overlay | Lists all shortcuts in a popover |
|
|
|
|
Shortcuts are displayed in a help popover (triggered by `?` key) anchored to the bottom-right of the review tab.
|
|
|
|
```
|
|
+------------------------------------+
|
|
| Review Shortcuts |
|
|
| ────────────── |
|
|
| n / Shift+N Next/prev thread |
|
|
| j / k Next/prev file |
|
|
| ] / [ Next/prev hunk |
|
|
| r Resolve thread |
|
|
| c Add comment |
|
|
| u Toggle view mode |
|
|
| f Toggle file tree |
|
|
| ? This help |
|
|
+------------------------------------+
|
|
```
|
|
|
|
---
|
|
|
|
## Source
|
|
|
|
- `packages/web/src/components/review/ReviewTab.tsx`
|
|
- `packages/web/src/components/review/DiffViewer.tsx`
|
|
- `packages/web/src/components/review/DiffViewerSplit.tsx` (proposed — split view variant)
|
|
- `packages/web/src/components/review/ReviewSidebar.tsx`
|
|
- `packages/web/src/components/review/ReviewFileTree.tsx` (proposed — file navigation pane)
|
|
- `packages/web/src/components/review/InlineCommentForm.tsx` (proposed — inline comment creation)
|
|
- `packages/web/src/components/review/HunkHeader.tsx` (proposed — hunk-level controls)
|
|
- `packages/web/src/components/review/PreviewPanel.tsx`
|
|
- `packages/web/src/components/review/FileCard.tsx`
|
|
- `packages/web/src/components/review/ReviewShortcutHelp.tsx` (proposed — keyboard shortcut overlay)
|
|
|
|
---
|
|
|
|
## Design Review Notes
|
|
|
|
This section documents design decisions, trade-offs, and open questions identified during review.
|
|
|
|
### 1. Sidebar width: fixed 300px was inadequate
|
|
|
|
**Problem**: A fixed 300px sidebar cannot accommodate long comments, code snippets in replies, or multi-reply threads without severe cramping. At 300px with padding, you get roughly 35 characters per line — barely enough for a sentence.
|
|
|
|
**Resolution**: Changed to resizable sidebar with drag handle. Default 320px, min 240px, max 480px. Width persists in localStorage. The drag handle (`‖`) uses `cursor-col-resize` and is 4px wide — thin enough to not waste space, wide enough to be a reliable grab target.
|
|
|
|
### 2. Jump-to-next button was ambiguous
|
|
|
|
**Problem**: The `[v]` icon (ChevronDown) alone is ambiguous — could mean "collapse", "dropdown", or "scroll down". Only having "next" with no "previous" forces users to cycle through all threads to go back one.
|
|
|
|
**Resolution**: Added both `[^]` (ChevronUp) and `[v]` (ChevronDown) with tooltips that include the keyboard shortcut. On wider viewports, text labels are shown: `[^ Prev] [Next v]`. Keyboard shortcuts `n` / `Shift+N` map to these.
|
|
|
|
### 3. Thread resolution had no visual feedback
|
|
|
|
**Problem**: Clicking "Resolve" with no documented visual transition leaves the user guessing — did it work? Where did the thread go?
|
|
|
|
**Resolution**: Documented a four-step animation: immediate badge swap, opacity fade (300ms), collapse to single-line summary (400ms), and gutter marker color change (amber to green). Also added `[Unresolve]` for reverting mistakes — resolution should not be a one-way door.
|
|
|
|
### 4. Inline comment creation was completely missing
|
|
|
|
**Problem**: The spec documented viewing and resolving threads but never explained how to CREATE one. This is the most fundamental interaction in a review workflow.
|
|
|
|
**Resolution**: Two creation paths documented: (a) click `[+]` gutter icon on any diff line, (b) select text and click floating `[Comment]` toolbar. Both open an inline form below the diff line. Gutter markers (colored dots) provide persistent visual indicators of where threads exist.
|
|
|
|
### 5. Multi-file navigation was absent
|
|
|
|
**Problem**: Real-world diffs span dozens of files. Without a file tree or file tabs, the user has no way to navigate between files or see which files need attention.
|
|
|
|
**Resolution**: Added a collapsible left pane (`w-48`) with a file tree. Files with unresolved threads are marked with `*` and sorted first. Addition/deletion counts are color-coded. Keyboard navigation with `j`/`k`.
|
|
|
|
### 6. No unified vs. split view toggle
|
|
|
|
**Problem**: Developers have strong preferences here. Unified is better for small changes; split is better for large refactors. Not offering both is a deal-breaker for power users.
|
|
|
|
**Resolution**: Added `[Unified | Split]` segmented toggle in the diff header. Split view wireframed with synced scroll, separate line numbers, and hatched empty-line fills. Preference persists in localStorage. Keyboard shortcut `u` for fast toggling.
|
|
|
|
### 7. "Agent still working" state was undocumented
|
|
|
|
**Problem**: The empty state covered "no changes" and the error state covered failures, but the common case of "agent is actively producing changes" was missing. Users would see a partial diff with no explanation.
|
|
|
|
**Resolution**: Added "Partial Diff State" section. Files being modified get a `~` prefix (active blue). A bottom banner with spinner explains the situation. Diff auto-refreshes as the agent works. Threads created on partial diffs are preserved across updates.
|
|
|
|
### 8. No per-hunk accept/reject
|
|
|
|
**Problem**: Thread resolution is per-comment, but code review also needs per-change-block controls. Without hunk-level accept/reject, users cannot efficiently signal "this chunk looks good" vs. "this chunk needs rework."
|
|
|
|
**Resolution**: Added `[Accept Hunk]` / `[Reject Hunk]` controls in each `@@` hunk header. Accept dims the hunk to signal "reviewed." Reject opens a confirmation and creates a revert suggestion thread. Controls are hover-revealed to keep the UI clean.
|
|
|
|
### 9. No keyboard shortcuts
|
|
|
|
**Problem**: The spec is for a "keyboard-first" mission control tool, but the review tab had zero keyboard shortcuts documented. Mouse-only review is slow and antithetical to the design stance.
|
|
|
|
**Resolution**: Full keyboard shortcut table added: `n`/`Shift+N` for thread navigation, `j`/`k` for file navigation, `]`/`[` for hunk navigation, `r` for resolve, `c` for comment, `u` for view toggle, `f` for file tree toggle. `?` opens a help overlay.
|
|
|
|
### 10. Syntax highlighting not specified
|
|
|
|
**Problem**: The diff viewer was plain text with only `+`/`-` coloring. No mention of syntax highlighting, despite the theme spec already defining terminal tokens suitable for code coloring.
|
|
|
|
**Resolution**: Added a syntax highlighting section mapping code elements to terminal tokens from `theme.md`. This ensures visual consistency between agent output and diff review. Important constraint documented: syntax colors on `color` only, never `background-color`, so diff add/remove backgrounds remain visible.
|