Skip to content

feat(chat): file attachments, accessibility, E2E test#68

Merged
mcheemaa merged 2 commits into
mainfrom
project-4-chat-polish
Apr 15, 2026
Merged

feat(chat): file attachments, accessibility, E2E test#68
mcheemaa merged 2 commits into
mainfrom
project-4-chat-polish

Conversation

@mcheemaa
Copy link
Copy Markdown
Member

Summary

Final PR for the Phantom chat channel (Project 4, PR4 of 4). After this, the chat is production-ready.

File attachments

  • Upload endpoint: multipart, 10MB per file, 10 file cap, type allowlist
  • HEIC explicitly rejected with iOS-specific message
  • Message builder converts attachments to SDK-native content blocks (ImageBlockParam, DocumentBlockParam, TextBlockParam)
  • Client: clipboard paste, drag-and-drop with full-bleed overlay, file picker via paperclip button
  • Thumbnail strip above textarea with upload progress

Keyboard shortcuts and command palette

  • Cmd+K: command palette with Conversations, Navigation, Help categories
  • Cmd+/: keyboard help sheet
  • Cmd+Shift+L: focus composer
  • IME composition guard for Japanese input (compositionstart/compositionend)

Accessibility

  • aria-live="polite" region announces streaming content to screen readers
  • Skip links: "Skip to main content" and "Skip to composer"
  • enterKeyHint="send" for mobile keyboards

E2E test

  • Playwright test gated on PHANTOM_E2E_URL (skips in CI without live agent)
  • Full flow: welcome, send, stream, refresh, resume, stop, partial preservation

Project 4 complete

PR What Status
#65 Backend foundations Merged
#66 Client scaffold and streaming Merged
#67 Auth, first-run, Web Push Merged
#68 Attachments, accessibility, E2E This PR

Total: ~13,500 LOC across 4 PRs. 1,584 tests, zero regressions.

Test plan

  • bun test: 1,584 pass, 0 fail
  • bun run typecheck and bun run lint: clean
  • chat-ui typecheck and build: clean, ~220KB gzipped
  • Manual: paste screenshot, verify model sees image
  • Manual: drag PDF, verify model summarizes it
  • E2E: PHANTOM_E2E_URL=https://cheeks.ghostwright.dev npx playwright test

Final PR for the chat channel. The chat is production-ready.

File attachments:
- Upload endpoint (multipart, 10MB limit, 10 file cap)
- Type allowlist: PNG, JPEG, GIF, WebP, PDF, text/code (HEIC rejected)
- Message builder converts to SDK-native ImageBlockParam,
  DocumentBlockParam, TextBlockParam
- Client: paste handler, drag-and-drop with overlay, file picker
- Thumbnail strip above textarea with progress indicators

Keyboard shortcuts and command palette:
- Cmd+K opens command palette (Conversations, Navigation, Help)
- Cmd+/ opens keyboard help sheet
- Cmd+Shift+L focuses composer
- Cmd+Shift+D toggles theme
- IME composition guard prevents premature send during Japanese input

Accessibility:
- aria-live="polite" region for streaming content
- Skip links: "Skip to main content" and "Skip to composer"
- enterKeyHint="send" for mobile keyboards

E2E test:
- Playwright test gated on PHANTOM_E2E_URL env var
- Full flow: welcome state, send message, verify streaming, refresh
  and verify history, stop and verify partial preservation

53 new tests, 1,584 total, zero regressions.
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2fcc2e02aa

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/chat/upload.ts
Comment on lines +116 to +120
deps.attachmentStore.create({
sessionId,
kind,
filename: sanitizeFilename(file.name),
mimeType: mime,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Use persisted attachment IDs in upload responses

processFiles returns the locally generated id to the client, but attachmentStore.create(...) generates and stores a different UUID internally. That means the client receives IDs that do not exist in chat_attachments, so preview URLs 404 and buildUserMessageParam cannot resolve uploaded attachments during /chat/stream. Return the store-generated ID (or let the store accept a caller-provided ID) so response IDs match persisted rows.

Useful? React with 👍 / 👎.

Comment on lines +129 to +131
setFiles((prev) =>
prev.map((f) => (f.status === "uploading" ? { ...f, status: "done" as const } : f)),
);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Treat non-2xx attachment uploads as failures

The upload client path marks every uploading file as done without checking res.ok, so 4xx/5xx responses (for example 413 with { error, message }) are treated as success. In the current send flow this causes attachments to be cleared and the message to be sent without them, silently dropping user files after a failed upload. Check HTTP status before success-state transitions and preserve files on failure.

Useful? React with 👍 / 👎.

P1 fixes:
- Attachment ID mismatch: store.create() accepts caller-provided ID
  so upload response IDs match DB rows
- Upload error handling: check res.ok, mark files as error on failure,
  show toast, do not clear files
- Stored XSS prevention: preview endpoint adds Content-Disposition
  (attachment for non-images), X-Content-Type-Options: nosniff,
  Content-Security-Policy: sandbox
- Orphan sweep: deleteAttachmentFile called before DB row deletion,
  session hard-delete also removes attachment files from disk

P2 fixes:
- Attachments cleared on session switch (clearFiles in sessionId effect)
- Client file size limits aligned per-type: 32MB PDF, 10MB image, 1MB text
- Stale closure race: filesRef for consistent snapshot in uploadFiles
@mcheemaa mcheemaa merged commit e080ab0 into main Apr 15, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant