feat: convert large pasted text into file attachments#23379
feat: convert large pasted text into file attachments#23379ibetitsmike merged 35 commits intomainfrom
Conversation
ibetitsmike
changed the title
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a9c869455f
ℹ️ 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".
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31850b96d7
ℹ️ 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".
|
@codex review |
1 similar comment
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
ibetitsmike
force-pushed
the
mike/text-paste-attachments
branch
from
2002641 to
ea36a3d
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea36a3d7ab
ℹ️ 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".
site/src/pages/AgentsPage/components/AgentDetail/ConversationTimeline.tsx
Outdated
Show resolved
Hide resolved
Documentation CheckUpdates Needed
Automated review via Coder Tasks |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 278cd61318
ℹ️ 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".
|
Re: large paste file parts dropped by model adapters — acknowledged. The current chat adapters only handle image/audio/pdf media types and will need to be updated to pass |
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 278cd61318
ℹ️ 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".
ibetitsmike
force-pushed
the
mike/text-paste-attachments
branch
from
b5137c0 to
07ccbf8
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 04e13d7887
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cdc74e9052
ℹ️ 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".
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cdc74e9052
ℹ️ 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".
ibetitsmike
force-pushed
the
mike/text-paste-attachments
branch
from
c6ddddd to
2837061
Compare
ibetitsmike
force-pushed
the
mike/text-paste-attachments
branch
from
2837061 to
fb61d04
Compare
johnstcn
left a comment
johnstcn
left a comment
There was a problem hiding this comment.
Deep Review Summary
+1225 / -181 across 16 files. The feature is well-scoped but has meaningful correctness bugs, accessibility regressions, and test coverage gaps.
Existing comment from @DanielleMaywood on pasteHelpers.test.ts ("We need to make the AGENT.md really emphasise that we should never do this") — refers to placing a standalone vitest .test.ts file inside components/ instead of utils/. Agreed.
P1 — Must-fix before merge
1. Stale-index race in "Paste inline" removes wrong attachment
site/src/pages/AgentsPage/components/AgentChatInput.tsx ~line 478
handleInlineText captures attachments from the render closure and computes attachments.indexOf(file) to pass to onRemoveAttachment. The "Paste inline" onClick is async — it awaits loadTextAttachmentContent before calling back. If the user removes another attachment during that await, the stale closure index targets the wrong item.
Concrete scenario: attachments are [A, B, C] → user clicks "inline" on B → async fetch starts → user removes A → state becomes [B, C] → fetch resolves → stale indexOf(B) returns 1 → handleRemoveAttachment(1) uses functional updater against current [B, C] → removes C instead of B.
suggestion: Pass File reference to removal instead of a numeric index, or resolve the index inside the functional state updater.
2. No abort/cancellation on loadTextAttachmentContent in composer
site/src/pages/AgentsPage/components/AgentChatInput.tsx ~line 284
fetchTextAttachmentContent already accepts an optional AbortSignal, and TextAttachmentButton in ConversationTimeline.tsx correctly creates an AbortController + cleans up on unmount. The composer's loadTextAttachmentContent never passes a signal. Fetch continues after unmount/removal.
3. "Paste inline" button is keyboard-inaccessible
site/src/pages/AgentsPage/components/AgentChatInput.tsx line 359
tabIndex={-1} removes this from tab order entirely. This is the only UI path to convert a text attachment back to inline text. The sibling Remove button (line 387) does NOT have tabIndex={-1}, so keyboard users can delete but not inline — an inconsistent a11y regression.
P2 — Should-fix before merge
4. Dead fileName parameter on onTextPreview
AgentChatInput.tsx lines 122, 270, 489 / TextPreviewDialog.tsx
onTextPreview signature includes fileName, threaded through the entire callback chain. But TextPreviewDialog has no fileName prop — its title is hardcoded as "Pasted text". Either wire it in or remove it.
5. Missing aria-label on composer text attachment button
AgentChatInput.tsx ~line 325
No aria-label; accessible name falls back to a truncated snippet. The timeline equivalent at ConversationTimeline.tsx line 130 correctly sets aria-label="View text attachment".
6. No focus-visible ring on text attachment buttons
AgentChatInput.tsx line 327 / ConversationTimeline.tsx line 131
Neither button has focus-visible: classes. Peer buttons in the same files (edit-message, context-indicator) correctly use focus-visible:ring-2 focus-visible:ring-content-link.
7. Silent error handling on fetch failures
AgentChatInput.tsx ~line 290 / ConversationTimeline.tsx ~line 178
Composer path is completely silent — catch { return undefined }, not even console.error. Timeline path logs but provides no user feedback. Click does nothing on failure.
8. fetchTextAttachment.ts pure functions have zero test coverage
site/src/pages/AgentsPage/utils/fetchTextAttachment.ts
formatTextAttachmentPreview and decodeInlineTextAttachment are trivially testable pure functions. Every other file in this utils/ directory (6/7) has a corresponding .test.ts.
9. Missing reverse MIME mismatch backend test
coderd/exp_chats_test.go
ContentSniffingRejects tests text-body-as-PNG. The reverse — valid PNG body declared as text/plain — is untested. Now that text/plain is in the allowlist, this is security-relevant.
10. Core paste→attachment integration is untested
site/src/components/ChatMessageInput/ChatMessageInput.tsx
PasteSanitizationPlugin wires paste event → isLargePaste → createPasteFile → onFilePaste. This is the core feature of the PR and has zero integration/story coverage. The Cmd/Ctrl+Shift+V bypass (plainTextPasteRef + 500ms setTimeout) is complex stateful logic, also untested.
P3 — Nice-to-have / follow-up
| # | File | Issue |
|---|---|---|
| 11 | pasteHelpers.test.ts |
Standalone vitest in components/ — should be in utils/ (Danielle's concern) |
| 12 | AGENTS.md |
Unrelated change (TS double-cast guideline) — separate PR |
| 13 | ConversationTimeline.stories.tsx |
spyOn(globalThis, "fetch") replaces all fetch in beforeEach; non-matching URLs → 404 |
| 14 | pasteHelpers.ts |
File.text() runtime polyfill for jsdom should live in test setup, not production code |
| 15 | Various .stories.tsx |
1/15 new stories has a play function; rest are render-only |
| 16 | useFileAttachments.ts |
URL.createObjectURL called for text/plain files but blob URL is never consumed |
Refuted (not real issues)
| Original claim | Why it's wrong |
|---|---|
isLargePaste trailing newline off-by-one |
Intentional — code has explicit comment documenting this; Cmd+Shift+V exists as escape hatch |
TextAttachmentButton stale cache on fileId change |
Component is React-keyed; fileId is structurally immutable per message block |
TextPreviewDialog min-w-[400px] overflows narrow viewports |
It's sm:min-w-[400px] — only applies ≥640px; correctly responsive |
🤖 This response was generated by Coder Agents.
ibetitsmike
force-pushed
the
mike/text-paste-attachments
branch
from
ddebac4 to
6942e06
Compare
5 participants
Summary
text/plainfile attachmentsValidation
pnpm -C site exec vitest run --project=unit src/pages/AgentsPage/utils/pasteHelpers.test.tspnpm -C site exec vitest run --project=storybook src/pages/AgentsPage/AgentChatInput.stories.tsxgit commit(pre-commit hook: fmt/lint/build)