refactor(chat): move ChatPage resource handling into a controller hook

[bolichen97]

Stack: top — stacked on #9072. Base is refactor/chat-page-session-controller, not main; read this diff relative to that base only. #8978 and #9079 are merged; remaining order: #9072 → this. After #9072 merges, the base is retargeted to main and the branch rebased.

Problem / Motivation

website/src/pages/ChatPage.tsx is a 10k-line page. About 650 lines of it manage the resources around a chat: side-panel tabs, message search, pull-request / issue source selection, opening files, folders, artifacts and diffs, saving a file, screen snips, uploads and drag-and-drop. None of it is about the transcript or the composer, but it all lives inline in the component.

Why it matters

The whole-page split in #7255 grew past the point where CI's Opus review lane can finish one pass and collided with main's chat-core transport work. It lands as slices that each fit one review pass. This slice owns resource ingress and the side panel. It does not touch the send path.

What changed (motivation → approach → change)

The moved code reads a handful of page values (the active slot, messages, the drafts refs, a few setters) and returns the handlers the page passes into the transcript and the composer. That is a custom hook.

ChatPage.tsx lines 3145–3564, 3584–3661 and 3707–3860 move into pages/chat/useChatPageResourcesController.tsx. The page calls useChatPageResourcesController({...}) at the spot the first moved line used to be and destructures the twenty-nine values it still reads. handleSubagentPanelOpen, sessionTitles, handleFork and handlePlanFromHere sat between the moved blocks and stay in the page, in the same order.

The moved text is main's bytes, with two mechanical edits and one declared behaviour fix (below). The page state the code reads arrives as hook arguments: a composer port (input ref and setter, draft refs, setPendingFiles, currentProjectRef, the voice caret refs, saveDrafts), a capture port (setUploading, setUploadError, setUploadHint, setResizedInfo, snipSlotRef, setSnipFrame) and showActionError. The dependency arrays name the refs and setters the hook now receives — react-hooks/exhaustive-deps cannot see them as stable once they arrive as arguments — and they are referentially stable, so no callback is recreated and no effect re-fires.

One behaviour change, declared. takeScreenshot's catch used to be bare and commented /* user cancelled */. That comment was wrong: POST /api/screenshot answers a cancelled screencapture -i with 200 {"path": ""}, which the if (path) guard already absorbs, so the only things that ever reached the catch were real failures — the 400 off macOS, the 120s capture timeout, or the request never reaching the gateway — and the user saw nothing. The catch now reports the server's own reason through showActionError (the hook's existing error port). This is the one line in the moved code that is not main's bytes. It is here rather than in a separate fix: PR because the moved line reads as an added line, and the errors-use-error-notice rule blocks an added bare catch; carrying the smallest possible fix, declared and tested, is the way to keep this slice reviewable as a move without a human override.

One declaration moves earlier: currentProjectRef is now created just before the hook call, because the call site passes it eagerly and it used to be declared 2,000 lines later. Its value is still slots.find(...)?.project computed every render, so what the handlers read at event time is unchanged.

Tests

• ChatPageMoreCoverage.test.tsx gains a ChatPage screen capture failures block (2 tests): a rejected api.screenshot() surfaces its message as an error notice; a cancelled capture ({ path: '' }) stays silent. The ChatInput mock exposes onScreenshot so the composer's capture button can be pressed.
• Every ChatPage* spec plus the panel-tabs, pins, source-selection, pull-request-links, upload-routing, drop-classify, screen-snip and message-search specs pass unchanged: 78 files / 946 tests targeted. Full suite result in the first comment.
• tsc -b, eslint (0 warnings), jscpd (0 clones), lint:phantom-classes, check_feature_map.py, check_brand_name.py, docs-lint.sh all exit 0.

Manual verification

N/A — the moved handlers are covered by the ChatPage.dirStaging, ChatPagePasteImage, ChatPage.sourceSelection, ChatPage.sidebarSourceReveal, ChatPage.handleOpenDiffNoop and ChatPage.gitPanelGate characterizations, which pass unchanged.

Screenshots / video

Why no screenshot: hook extraction; the only rendered change is that a failed screen capture now shows the existing ErrorNotice with the server's message where before it showed nothing — no new component, layout or copy beyond the passed-through reason.

Related Issues

Part of the split of #7255. no linked issue: the parent PR has no tracking issue.

Checklist

• At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable)
• No secrets, credentials, or internal references in the diff

[github-actions]

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of e1d96ced99220f0805a5625279153772fdf89781 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All verification checks pass. I have everything needed for the verdict.

The backend confirms the rider fix's premise: files.py:1506 answers a cancelled capture with 200 {"path": ""}, while real failures return 400/504 — so the old bare catch commented "user cancelled" was indeed swallowing only genuine errors. The errors-use-error-notice rule is real and blocking: true (website/AUTOSDE.yaml:526). The hook is the third sibling of an established split mechanism (useChatPageSessionController, useChatPageTranscriptController both exist). All 29 destructured return values have real uses in ChatPage.tsx beyond the destructure (counted: minimum 2 occurrences each, search at 81). The moved blocks are byte-faithful to the removed ones apart from the declared edits.

First-Principles-Verdict: PASS

A faithful move of 650 inline lines into the established controller-hook split, plus one declared, backend-grounded fix for silently swallowed screenshot failures.

What this change ships

Intent: make ChatPage's resource handling reviewable by extracting it into a hook, as one slice of the #7255 split — a MOVE (refactor), not a fix or addition.

1. Resource/side-panel/upload code moves from ChatPage.tsx into useChatPageResourcesController.tsx — justified (CI review-lane size limit; third sibling of an existing split mechanism)
2. A failed screen capture now shows an error notice with the server's reason — rides along, declared, derived (backend files.py:1506 proves cancel is 200 {"path":""}, so the old catch hid only real failures; blocking errors-use-error-notice rule bars re-adding the bare catch)
3. New i18n key screenshot_failed_reason across 13 locales — justified (required by item 2 + i18n gates)
4. Feature-map row names the new file — justified (same-commit doc invariant)
5. Two tests pin failure-reported / cancel-silent — justified (covers item 2)
6. currentProjectRef declared 2,000 lines earlier, same value — declared, mechanical
7. Hook options surface (composer/capture ports), one consumer — inherent to the split pattern, same as both sibling controllers; not premature generalization

Consumer count on the new surface: every one of the 29 returned values has ≥1 real use in ChatPage.tsx (grepped each name; minimum 2 occurrences including the destructure). Zero dead exports. The rider fix's zero option is a user clicking Screenshot and seeing nothing on a real 400/504 — a named, nonzero harm — so it earns its ride.

[FIRST-PRINCIPLES-REVIEWED] e1d96ce

[github-actions]

UX Review (Fable 5) — 🟡 CONCERNS

UX-level review of e1d96ced99220f0805a5625279153772fdf89781 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All evidence is in. The diff is a pure hook extraction with one declared user-visible change: a failed screen capture now surfaces "Screenshot failed: {{reason}}" through the page's existing showActionError notice — the exact formula its siblings (fork_failed_error, send_failed_with_error, could_not_read_file_reason) already use, with the same unknown_error fallback. No JSX, layout, control, or other copy changes; the blind read did not run because no screenshots were committed, so that one new rendered state is unwitnessed.

UX-Verdict: CONCERNS

Pure hook extraction with one new rendered state — the screen-capture failure notice — that no screenshot shows and no blind reader saw.

Evidence gaps

• The new error state "Screenshot failed: {{reason}}" (useChatPageResourcesController.tsx, takeScreenshot catch) renders through the existing ErrorNotice, but appears in no committed screenshot and the blind read did not run; one screenshot of the notice with a real server reason (e.g. the non-macOS 400) would close the gap. Everything else in the diff is a byte-for-byte move with no visual delta, so this is the only unwitnessed surface.

[UX-REVIEWED] e1d96ce

[github-actions]

Design Review (Fable 5) — ✅ PASS

Design-level review of e1d96ced99220f0805a5625279153772fdf89781 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

The diff is a faithful move: the hook body is main's bytes with the declared single-line catch fix, tests and i18n keys back the fix, the feature-map doc is updated in the same commit, and the page-side wiring matches the description exactly. The extraction follows the established useChatPageSessionController/useChatPageTranscriptController precedent, changes no public surface, and is trivially reversible.

Design-Verdict: PASS

A mechanical, well-declared slice following the existing controller-hook precedent; the one behavior change is documented, justified, and characterization-tested.

[DESIGN-REVIEWED] e1d96ce

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of e1d96ced99220f0805a5625279153772fdf89781 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] e1d96ce

False positive or not applicable? A repository writer can comment:
/ai-review override gpt e1d96ced99220f0805a5625279153772fdf89781: <one-sentence reason>

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed e1d96ced99220f0805a5625279153772fdf89781 — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] e1d96ce

Verdict parsed from the review's SHA-scoped output markers for commit e1d96ced99220f0805a5625279153772fdf89781.

False positive or not applicable? A repository writer can comment:
/ai-review override fable e1d96ced99220f0805a5625279153772fdf89781: <one-sentence reason>

[bolichen97]

Local gate run on the stack top e01d1beb9 (= #8978 + #9072 + #9078 + #9079 together, i.e. this PR's merge result):

• Full website vitest suite with coverage: 1887 files, 29707 passed, 1 expected fail, 2 skipped, exit 0.
• Frontend line-rate 92.63% (floor 90). scripts/check_per_file_coverage.py (floor 80, frontend baseline): exit 0 — src/pages/ChatPage.tsx 80.89%, ChatPageMessageContent.tsx 90.31%, useChatPageSessionController.ts 98.96%, useChatPageResourcesController.tsx 85.34%, useChatPageTranscriptController.tsx 98.79% (de-duplicated line counts, the gate's own measure).
• Per PR on its own base: tsc -b 0 errors outside src/stories/, eslint 0 warnings, every ChatPage* spec green (65 files / 587 tests), check_feature_map.py with FEATURE_MAP_BASE_REF set to the base branch exit 0.

The base branch was changed.

[bolichen97]

The merge conflict label on this PR is stale — the conflict is resolved. It was
real while this branch still sat on refactor/chat-page-session-controller,
which #9072's squash (9e497f7c4) replaced when it merged at 01:03Z.

Rebased onto main as 662ee2c12, and GitHub now reports the branch clean:

check	value
compare/main...662ee2c12	ahead_by=1, behind_by=0, merge_base=cbdd4a569
mergeable	false → true
commits	1 (the redundant #9072 commit was dropped by the rebase)

Please don't close or revert on the strength of that label; the labeller just
hasn't caught up. Content is unchanged from f39308104 apart from the new base.

Gates on this exact tree: eslint clean, tsc -b with zero introduced errors,
83 files / 783 tests across the ChatPage/ChatPane specs, and the feature-map gate
passes against main. This is also the first time this slice gets the full
62-lane matrix — every earlier green here was the 17-lane subset that runs when
the base is a branch rather than main — so the backend, E2E and Coverage Gate
results now arriving are new information rather than a re-run.

Still open and not addressed by the rebase: GPT's two errors-use-error-notice
findings (useChatPageResourcesController.tsx:163, :576). Both are main's
existing shapes (ChatPage.tsx:2452-2458 and :2883) surfaced because a move
makes every relocated line an added one, so they need a scope decision rather
than another round on this branch.

[bolichen97]

Raw exception text as the whole message — showActionError(errMessage(e) || …) puts unprefixed transport text above the composer, naming no action — accepted, deferred to #9186 with the wording fix it belongs to.

the user who clicked the camera can't tell what failed. Fix: wrap the reason like the sibling could_not_read_file_reason does, e.g. a "Screenshot failed: {reason}" catalog key.

Correct, and the reasoning is right: "screenshot timed out" with no subject is a
bare string, not a notice. The sibling pattern is the one to copy.

Being plain about why it reads that way: minting a catalog key means touching the
locale files the i18n gate requires, and I judged that too much surface to add
inside an 826-line move. That trade landed on the wrong side — the point of the
change was to stop a failure being invisible, and a message that names no action
only half does that.

It is deferred rather than fixed here for a reason that is about this PR's
budget, not about preferring the narrow diff:

• The change is still a strict improvement on main, where this failure was
  entirely silent (} catch { /* user cancelled */ }, whose comment was wrong
  — a cancelled capture returns 200 {"path": ""} and never reaches the catch).
  So nothing regresses by landing it as-is.
• The wording belongs with the rest of the cluster. chat: close the silent-failure cluster in ChatPage's resource region as a class, not per slice #9186 collects every silent
  failure path in this region — the source-host useQuery with no error path,
  the artifact-reference .catch, three further catch sites — and mints one
  shared "<action> failed: {reason}" key for all of them. Doing this one alone
  now would mint a key that chat: close the silent-failure cluster in ChatPage's resource region as a class, not per slice #9186 then has to generalise.
• This slice is a declared byte-faithful move that already drew a First
  Principles BLOCK and a Design CONCERNS for carrying one behavioural edit. A
  second one, plus locale files, is more than one review pass should hold.

Evidence gap — the new error state appears in no committed screenshot, and the <!-- no-visual-delta --> claim is contradicted — accepted; the claim is already corrected, the screenshot rides with the wording fix.

Close with one screenshot of the notice after a failed capture.

The description no longer claims no visual delta; it names the rendered
difference explicitly. On the screenshot itself: capturing it now would document
a string #9186 is going to reword, so the evidence would be stale before it is
useful. It is listed in #9186 alongside the wording change so the frame shows
the message that actually ships.

[bolichen97]

Raw exception text as the whole message — showActionError(errMessage(e) || …) names no action — fixed in aaaacb5b. This supersedes my earlier disposition, which deferred it on a cost I had not measured.

Fix: wrap the reason like the sibling could_not_read_file_reason does, e.g. a "Screenshot failed: {reason}" catalog key.

Done exactly that. New key pages.chatPage.screenshot_failed_reason =
"Screenshot failed: {{reason}}", on the same shape as the existing
send_failed_with_error and could_not_read_file_reason.

Correcting myself on why it was deferred. I said minting a key meant touching
the locale files the i18n gate requires, and judged that too much surface for a
move PR. I never measured it. It is two files: en.manual.json plus a
regenerated en-XA.json (npm run i18n:pseudo). The eleven translated catalogs
are not required — [source-strings] accepts a new English key, and
[changed-passthrough] is zero-tolerance on values that still read as English,
so copying English into them would have failed the gate rather than satisfied
it. Missing keys fall back to English at runtime and translators pick them up on
their own cycle. The whole review-pass argument rested on a number I guessed, so
the right answer was to fix it here.

Verification on this head: i18n:check 19/19 PASS against origin/main, eslint
clean, tsc -b with no introduced errors, 87 files / 880 tests. The test now pins
the composed message rather than the bare reason, and it is mutation-checked —
reverting the call site to errMessage(e) fails exactly that assertion, and the
control passes again once restored.

Evidence gap — the notice appears in no committed screenshot — accepted, tracked in #9186.

The <!-- no-visual-delta --> claim is already gone from the description, which
now names the rendered difference. The screenshot itself is listed in #9186: this
notice renders through the shared ErrorNotice component with no new styling, so
what a frame would document is the string, and #9186 is where the remaining
silent-failure sites in this region get the same "<action> failed: {reason}"
treatment. One capture taken there covers the whole family instead of this PR
shipping a frame of one member of it.

[bolichen97]

Evidence gap — the screen-capture failure notice renders in no committed screenshot and the blind read did not run — accepted, and deferred to #9186 on review-pass budget rather than on capability.

one screenshot of the notice with a real server reason (e.g. the non-macOS 400) would close the gap

Taking the earlier half first: the wording objection from the previous round is
resolved in this head. The notice now reads Screenshot failed: {{reason}}
through a new catalog key, translated in all eleven locales, with the composed
message pinned by a mutation-checked test. This disposition is only about the
frame.

I can produce it — this is not a "cannot". Playwright 1.58.2 is present and
chromium launches in this environment. What the repo's convention actually costs,
measured against scripts/capture-error-notice-chat-1.mjs, is: a new capture
scene under public/capture/, a new scripts/capture-*.mjs runner, a vite server
run, and one or two committed PNGs under temp-screenshots/ — two new files plus
images, on top of an 826-line relocation.

Why it lands in #9186 instead. This slice is a declared byte-faithful move
that already drew a First Principles BLOCK and a Design CONCERNS for carrying one
behavioural edit; it has since taken a second (the message key) and a third (an
i18n-forced reshape), each declared in the commit. A fourth addition that is a new
test harness rather than product code is past what one review pass should hold —
which is the criterion I was asked to decide this on.

The frame is also worth more there than here. #9186 collects the remaining silent
failures in this same region — the source-host useQuery with no error path, the
artifact-reference .catch, three further catch sites — and gives them the same
"<action> failed: {reason}" treatment. One capture scene added with that change
photographs the whole family in one pass, whereas a scene added now photographs
one member of it and then has to be extended anyway.

What stands behind the claim in the meantime, so the gap is not merely noted:

• the notice renders through the existing ErrorNotice component with no new
  styling, no new variant and no layout change — the diff adds a call site, not a
  surface;
• the composed string is asserted end-to-end (Screenshot failed: screenshot timed out) through a real page render, and the assertion is mutation-checked:
  reverting the call site to the bare reason fails it, and reverting the guard so
  a cancelled capture reports fails its sibling;
• catalogParity covers the key in all eleven locales, and the i18n gate's
  [changed-values] QA pass covers the translated values.

I have added the frame to #9186's scope explicitly rather than leaving it implied.