tas/Codewalkers
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity
Files
c52fa8654270eb5e9517fbfc26884daeb3750081
Codewalkers/docs/wireframes/v2/review-tab.md
Lukas May 1e374abcd6 docs: Design review pass on all v2 wireframes 
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)
2026-03-02 19:36:26 +09:00

28 KiB
Raw Blame History

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]                  |
+----------------------------------+
  1. Collapsed resolved threads can be expanded by clicking [v] (ChevronRight) to reveal full content
  2. The corresponding diff gutter marker transitions from bg-status-warning-dot to bg-status-success-dot
  3. 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
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.

Reference in New Issue View Git Blame Copy Permalink