fix(comprehension): apply second-pass code-review patches for story 3-2

- P1: remove custom onSeparatorKeyDown (react-resizable-panels v4 provides
  a native 5% keyboard step natively via keydown listener); drop 3 vacuous
  tests and forward onKeyDown in the test mock so store/DOM drift is gone
- P2: refine DiffViewer.isBinaryFile heuristic — exclude renames/copies,
  document mode-only false positive as deferred (gitdiff-parser's isBinary
  flag is empirically never set)
- P3: add 422/429 regression tests to ChatView
- P4: add role=region + aria-label to the binary-file placeholder
- P5: replace "Story 3.3 backlog" prose with grep-able TODO(story-3.3)

make lint + pytest + vitest all green (213 API + 24 web tests).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Author: mariuspruvot

apps/web/src/features/session/ChatView.test.tsx

@@ -133,6 +133,36 @@ describe('ChatView — data loading', () => {
     })
   })
 
+  test('renders 422 error screen with no Retry button when fetch returns 422', async () => {
+    mockApiFetchOnce(mockedApiFetch, new Response(null, { status: 422 }))
+
+    renderAtSessionRoute(<ChatView />)
+
+    await waitFor(() => {
+      expect(screen.getByText('Invalid session ID')).toBeTruthy()
+    })
+    expect(
+      screen.getByText(/This session link is malformed\. Check the URL and try again\./),
+    ).toBeTruthy()
+    // 422 is a URL problem — retrying would not help, so no Retry button.
+    expect(screen.queryByRole('button', { name: /retry/i })).toBeNull()
+  })
+
+  test('renders 429 error screen with no Retry button when fetch returns 429', async () => {
+    mockApiFetchOnce(mockedApiFetch, new Response(null, { status: 429 }))
+
+    renderAtSessionRoute(<ChatView />)
+
+    await waitFor(() => {
+      expect(screen.getByText('Rate limit exceeded')).toBeTruthy()
+    })
+    expect(
+      screen.getByText(/Too many requests\. Please wait a moment before trying again\./),
+    ).toBeTruthy()
+    // 429 deliberately has no Retry button — hammering the limiter is worse.
+    expect(screen.queryByRole('button', { name: /retry/i })).toBeNull()
+  })
+
   test('renders retryable error screen on 500 and Retry triggers a new fetch', async () => {
     // useSession retries 5xx twice (failureCount < 2), so 3 total attempts
     // before the query enters error state. We queue one extra 500 for the

apps/web/src/features/session/ChatView.tsx

@@ -125,6 +125,30 @@ export default function ChatView() {
       )
     }
 
+    // 422 → malformed session id. Retry is meaningless (the URL is the
+    // problem, not the server), so we omit the Retry button entirely.
+    if (status === 422) {
+      return (
+        <ErrorScreen
+          title="Invalid session ID"
+          message="This session link is malformed. Check the URL and try again."
+        />
+      )
+    }
+
+    // 429 → rate limit. A Retry button would hammer the limiter; instead we
+    // tell the user to wait.
+    // TODO(story-3.3): plumb the Retry-After header through SessionFetchError
+    // and render a live countdown here instead of the generic wait message.
+    if (status === 429) {
+      return (
+        <ErrorScreen
+          title="Rate limit exceeded"
+          message="Too many requests. Please wait a moment before trying again."
+        />
+      )
+    }
+
     return (
       <ErrorScreen
         title="Temporarily unavailable"

apps/web/src/features/session/DiffViewer.test.tsx

@@ -52,4 +52,29 @@ describe('DiffViewer', () => {
       'Large PR — diff truncated at 1 MB. Story 3.5 will add file-ranked selection.',
     )
   })
+
+  test('keeps binary files in the tab list with a placeholder body instead of silently dropping them', () => {
+    // Mixed PR: one text file + one binary file. git-format binary delta
+    // uses the `Binary files … and … differ` sentinel with zero hunks; the
+    // old code filtered it out and the user never knew the PR had a binary
+    // change. The fix keeps the tab and renders a placeholder body when
+    // the binary tab is active.
+    const mixedDiff = `${MULTI_FILE_DIFF}diff --git a/assets/logo.png b/assets/logo.png
+index 1111111..2222222 100644
+Binary files a/assets/logo.png and b/assets/logo.png differ
+`
+    render(<DiffViewer session={makeSession({ diff: mixedDiff })} />)
+
+    // Both text files AND the binary file are in the tab list.
+    expect(screen.getByTestId('diff-file-tab-0').textContent).toBe('foo.ts')
+    expect(screen.getByTestId('diff-file-tab-1').textContent).toBe('bar.py')
+    expect(screen.getByTestId('diff-file-tab-2').textContent).toBe('logo.png')
+
+    // Clicking the binary tab surfaces the placeholder, not a crash or
+    // empty state.
+    fireEvent.click(screen.getByTestId('diff-file-tab-2'))
+    const placeholder = screen.getByTestId('diff-binary-placeholder')
+    expect(placeholder.textContent).toContain('Binary file — not displayed')
+    expect(placeholder.textContent).toContain('assets/logo.png')
+  })
 })

apps/web/src/features/session/DiffViewer.tsx

@@ -21,16 +21,56 @@ function fileBasename(file: FileData): string {
   return fullPath.split('/').pop() || fullPath
 }
 
+// A binary delta parses into a FileData with zero hunks, a real path, and
+// `type: 'modify'`. The empty-input placeholder that `parseDiff` also emits
+// has neither hunks nor a meaningful path, so we can distinguish them by
+// checking for a usable path.
+//
+// ⚠️ `gitdiff-parser` technically defines a `file.isBinary` boolean on its
+// File type, but its internal parser NEVER sets it in practice: the branch
+// that does is only reached when the outer loop reads a line starting with
+// "Binary" directly, whereas the inner `simiLoop` (which processes the
+// headers following `diff --git`) aggressively consumes the `Binary files
+// … differ` line without matching any case — verified empirically with
+// `node -e "console.log(require('gitdiff-parser').parse(…).map(f => f.isBinary))"`.
+// Until the upstream parser fixes that, we fall back to the structural
+// check below.
+function hasRealPath(file: FileData): boolean {
+  const newPath = file.newPath && file.newPath !== '/dev/null' ? file.newPath : ''
+  const oldPath = file.oldPath && file.oldPath !== '/dev/null' ? file.oldPath : ''
+  return newPath !== '' || oldPath !== ''
+}
+
+function isBinaryFile(file: FileData): boolean {
+  // Exclude pure renames (`type === 'rename'`) and pure copies (`type ===
+  // 'copy'`) — they also parse with zero hunks + real paths but are clearly
+  // not binary. Mode-only changes (chmod) parse with `type: 'modify'` and
+  // zero hunks, so they WILL be mis-labelled as "Binary file — not
+  // displayed"; this is an accepted edge case (rare in practice, recoverable
+  // mis-label rather than a crash), tracked in `deferred-work.md`.
+  return (
+    file.hunks.length === 0 &&
+    hasRealPath(file) &&
+    file.type !== 'rename' &&
+    file.type !== 'copy'
+  )
+}
+
 export default function DiffViewer({ session }: DiffViewerProps) {
   const activeFileIndex = useSessionStore((s) => s.activeFileIndex)
   const setActiveFile = useSessionStore((s) => s.setActiveFile)
 
   // parseDiff always returns at least one file even for empty/garbage input
-  // (empty path, zero hunks). Treat such placeholders as "no changes" so
-  // the user sees the empty state instead of an empty tab bar.
+  // (empty path, zero hunks). Treat such empty placeholders as "no changes"
+  // so the user sees the empty state instead of an empty tab bar — but
+  // KEEP binary entries (zero hunks + real path + non-rename/copy) so a PR
+  // that only changes images / lockfiles is still discoverable from the tab
+  // list, with a "Binary file — not displayed" placeholder in the panel
+  // body. Pure renames and copies are dropped from the tab list (they have
+  // no content to show anyway).
   const files = useMemo<FileData[]>(() => {
     const parsed = parseDiff(session.diff)
-    return parsed.filter((file) => file.hunks.length > 0)
+    return parsed.filter((file) => file.hunks.length > 0 || isBinaryFile(file))
   }, [session.diff])
   // Anchor the check to the end of the diff — `includes()` would false-positive
   // on any file whose contents legitimately contain the marker comment (e.g.
@@ -160,7 +200,21 @@ export default function DiffViewer({ session }: DiffViewerProps) {
         className="flex-1 min-h-0 overflow-auto bg-surface"
         data-testid="diff-scroll-container"
       >
-        {activeFile && (
+        {activeFile && isBinaryFile(activeFile) && (
+          <div
+            className="h-full w-full flex items-center justify-center p-6"
+            data-testid="diff-binary-placeholder"
+            role="region"
+            aria-label={`Binary file: ${fileDisplayPath(activeFile)}`}
+          >
+            <p className="text-[14px] text-text-muted text-center">
+              Binary file — not displayed.
+              <br />
+              <span className="text-[12px]">{fileDisplayPath(activeFile)}</span>
+            </p>
+          </div>
+        )}
+        {activeFile && !isBinaryFile(activeFile) && (
           <Diff
             key={`${activeFile.oldRevision}-${activeFile.newRevision}-${safeIndex}`}
             viewType="unified"

apps/web/src/features/session/SplitLayout.tsx

@@ -12,9 +12,18 @@ const CHAT_PANEL_ID = 'session-chat-panel'
 const DIFF_PANEL_ID = 'session-diff-panel'
 
 // NOTE — react-resizable-panels v4 API (`Group`/`Separator`) replaced the
-// v2 `PanelGroup`/`PanelResizeHandle` mentioned in the story. The semantics
-// match: `Separator` still renders with `role="separator"` and handles
-// ArrowLeft/ArrowRight via keyboard out of the box.
+// v2 `PanelGroup`/`PanelResizeHandle` mentioned in the story. v4's
+// `Separator` already honours AC #3 without any custom code: it installs a
+// native DOM `keydown` listener on the separator element and steps the
+// layout by 5% on ArrowLeft / ArrowRight (verified at
+// `node_modules/react-resizable-panels/dist/react-resizable-panels.js`
+// lines 966/970 — the step is hardcoded to `H(t, ±5)`). The library's
+// handler also calls `event.preventDefault()` unconditionally, so Cmd+Arrow
+// / Alt+Arrow / Shift+Arrow passthrough is NOT supported by the library;
+// we do not attempt to work around that here because fighting the library's
+// internal listener via capture-phase `stopImmediatePropagation` is a
+// fragile coupling to v4 internals. Drag still bounds-checks via `minSize`
+// / `maxSize` on the `Panel`s and the store's own `[0.3, 0.8]` clamp.
 export default function SplitLayout({ session }: SplitLayoutProps) {
   const panelRatio = useSessionStore((s) => s.panelRatio)
   const setPanelRatio = useSessionStore((s) => s.setPanelRatio)