Add more dedicated e2e tests for file paste in chat input#329
Open
Add more dedicated e2e tests for file paste in chat input#329
Conversation
WalkthroughExtracted the paste-related e2e test from the main chat spec into a new dedicated test file and added a CHANGELOG entry; the original paste test was removed from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/frontend/apps/e2e/__tests__/app-conversations/file-paste.spec.ts (2)
128-145: Give the uploads-disabled case its own setup.This test reloads the app after the suite-level setup already booted it with uploads enabled. Moving it to a dedicated
describe/beforeEachwould keep the scenario isolated and less dependent on prior startup state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/apps/e2e/__tests__/app-conversations/file-paste.spec.ts` around lines 128 - 145, The test "pasting a file when upload is disabled does nothing" is relying on suite-level startup with uploads enabled; isolate it by creating a dedicated describe block (e.g., describe('uploads disabled')) and move this test inside with its own beforeEach that calls overrideConfig(page, { FEATURE_FLAGS: { 'document-upload': 'disabled', ... } }) and then page.goto('/'); ensure the test uses the same selectors (chatInput via getByRole) but does not depend on prior state; this keeps overrideConfig and page navigation local to that describe and guarantees uploads are disabled for this test.
232-256: Extract a shared paste dispatcher for the text case.This block reimplements the clipboard event stub separately from
pasteFile. A common helper for dispatching paste events would keep the file and text scenarios aligned as the component’sclipboardDatahandling evolves.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/apps/e2e/__tests__/app-conversations/file-paste.spec.ts` around lines 232 - 256, Extract the duplicated clipboard-event construction into a shared helper and use it in this test instead of reimplementing the stub; specifically, create a function (e.g., dispatchPasteEvent or pasteText) that mirrors the structure used by the existing pasteFile helper (building an Event('paste'), defining clipboardData with files/items/types/getData, and dispatching it on a given element) and replace the inline block in file-paste.spec.ts with a call to that helper so text and file paste logic stay aligned as clipboardData handling evolves.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/frontend/apps/e2e/__tests__/app-conversations/file-paste.spec.ts`:
- Around line 19-23: The test currently silently returns when the chat textarea
selector fails (the code using the variable textarea and selector
'textarea[name="inputchat-textarea"]'), which hides selector regressions; change
the early-return to a hard failure by throwing an Error (or using an assertion)
with a clear message like "Chat textarea not found:
'textarea[name=\"inputchat-textarea\"]' - selector mismatch or UI change" so
pasteFile/test fails fast and points to the broken selector.
---
Nitpick comments:
In `@src/frontend/apps/e2e/__tests__/app-conversations/file-paste.spec.ts`:
- Around line 128-145: The test "pasting a file when upload is disabled does
nothing" is relying on suite-level startup with uploads enabled; isolate it by
creating a dedicated describe block (e.g., describe('uploads disabled')) and
move this test inside with its own beforeEach that calls overrideConfig(page, {
FEATURE_FLAGS: { 'document-upload': 'disabled', ... } }) and then
page.goto('/'); ensure the test uses the same selectors (chatInput via
getByRole) but does not depend on prior state; this keeps overrideConfig and
page navigation local to that describe and guarantees uploads are disabled for
this test.
- Around line 232-256: Extract the duplicated clipboard-event construction into
a shared helper and use it in this test instead of reimplementing the stub;
specifically, create a function (e.g., dispatchPasteEvent or pasteText) that
mirrors the structure used by the existing pasteFile helper (building an
Event('paste'), defining clipboardData with files/items/types/getData, and
dispatching it on a given element) and replace the inline block in
file-paste.spec.ts with a call to that helper so text and file paste logic stay
aligned as clipboardData handling evolves.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 31a472c6-a663-41fc-8df7-d7a28d15df53
📒 Files selected for processing (3)
CHANGELOG.mdsrc/frontend/apps/e2e/__tests__/app-conversations/chat.spec.tssrc/frontend/apps/e2e/__tests__/app-conversations/file-paste.spec.ts
💤 Files with no reviewable changes (1)
- src/frontend/apps/e2e/tests/app-conversations/chat.spec.ts
providenz
force-pushed
the
providenz/paste-e2e
branch
from
b8faa18 to
1fce926
Compare
Signed-off-by: Laurent Paoletti <lp@providenz.fr>
providenz
force-pushed
the
providenz/paste-e2e
branch
from
1fce926 to
8fa22aa
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/frontend/apps/e2e/__tests__/app-conversations/file-paste.spec.ts (3)
65-67: Consider using a more stable selector for the chat input.The partial accessible name
'Enter your message or a'is fragile—any change to the placeholder text will break these tests. Adata-testidattribute or the same selector used inpasteFile(textarea[name="inputchat-textarea"]) would be more resilient.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/apps/e2e/__tests__/app-conversations/file-paste.spec.ts` around lines 65 - 67, The test uses a fragile partial accessible name when locating the chat input (chatInput via page.getByRole('textbox', { name: 'Enter your message or a' })); replace this with a stable selector such as the existing textarea[name="inputchat-textarea"] or a dedicated data-testid used elsewhere (or add one) and update the selector in this spec (and any helpers like pasteFile) to use that attribute instead of the partial placeholder text.
128-157: Route handler stacking is subtle but works here.Calling
overrideConfigtwice (once inbeforeEach, again here) stacks route handlers. Playwright invokes matched handlers in LIFO order, so the second call's disabled config takes precedence—which is the intended behavior. The re-navigation viapage.goto('/')ensures the app fetches the updated config.This works correctly, but the implicit reliance on handler ordering can be confusing. Consider adding a brief comment explaining why the second
overrideConfigcall overrides the first, or usingpage.unroute()before re-routing for explicitness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/apps/e2e/__tests__/app-conversations/file-paste.spec.ts` around lines 128 - 157, The test relies on calling overrideConfig a second time to change feature flags and then page.goto('/') to re-fetch config, but this stacking of Playwright route handlers (LIFO) is subtle; update the test to either add a brief inline comment clarifying that overrideConfig is intentionally called again because Playwright handlers are invoked LIFO so the second handler overrides the first, or explicitly clear previous routes before re-registering (e.g., call page.unroute or an equivalent cleanup) prior to calling overrideConfig to make the override unambiguous; reference overrideConfig, beforeEach, and page.goto('/') when making the change.
232-236: Same silent-failure pattern aspasteFile.This inline
page.evaluateblock has the same issue at line 236—silently returning when the textarea isn't found. For consistency with the suggested fix inpasteFile, throw an error here as well.Proposed fix
const textarea = document.querySelector( 'textarea[name="inputchat-textarea"]', ) as HTMLTextAreaElement; - if (!textarea) return; + if (!textarea) { + throw new Error( + 'Chat textarea not found: selector "textarea[name=\\"inputchat-textarea\\"]" may have changed', + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/apps/e2e/__tests__/app-conversations/file-paste.spec.ts` around lines 232 - 236, The inline page.evaluate callback in the file-paste.spec.ts test silently returns when the textarea isn't found; change that behavior to throw an error instead so the test fails loudly. Locate the page.evaluate block (the anonymous function that queries 'textarea[name="inputchat-textarea"]') and replace the early return with throwing a clear Error (e.g., "textarea not found") so failures mirror the pasteFile fix and surface in the test runner.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/frontend/apps/e2e/__tests__/app-conversations/file-paste.spec.ts`:
- Around line 65-67: The test uses a fragile partial accessible name when
locating the chat input (chatInput via page.getByRole('textbox', { name: 'Enter
your message or a' })); replace this with a stable selector such as the existing
textarea[name="inputchat-textarea"] or a dedicated data-testid used elsewhere
(or add one) and update the selector in this spec (and any helpers like
pasteFile) to use that attribute instead of the partial placeholder text.
- Around line 128-157: The test relies on calling overrideConfig a second time
to change feature flags and then page.goto('/') to re-fetch config, but this
stacking of Playwright route handlers (LIFO) is subtle; update the test to
either add a brief inline comment clarifying that overrideConfig is
intentionally called again because Playwright handlers are invoked LIFO so the
second handler overrides the first, or explicitly clear previous routes before
re-registering (e.g., call page.unroute or an equivalent cleanup) prior to
calling overrideConfig to make the override unambiguous; reference
overrideConfig, beforeEach, and page.goto('/') when making the change.
- Around line 232-236: The inline page.evaluate callback in the
file-paste.spec.ts test silently returns when the textarea isn't found; change
that behavior to throw an error instead so the test fails loudly. Locate the
page.evaluate block (the anonymous function that queries
'textarea[name="inputchat-textarea"]') and replace the early return with
throwing a clear Error (e.g., "textarea not found") so failures mirror the
pasteFile fix and surface in the test runner.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 391fa347-9397-4e7a-8368-4172b68d6671
📒 Files selected for processing (3)
CHANGELOG.mdsrc/frontend/apps/e2e/__tests__/app-conversations/chat.spec.tssrc/frontend/apps/e2e/__tests__/app-conversations/file-paste.spec.ts
💤 Files with no reviewable changes (1)
- src/frontend/apps/e2e/tests/app-conversations/chat.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/frontend/apps/e2e/__tests__/app-conversations/file-paste.spec.ts`:
- Around line 187-209: The test "pasting multiple different files shows all
attachments" currently simulates two separate paste events; change it to
dispatch both files in a single paste event by extending or overloading the
pasteFile helper (pasteFile) to accept an array of file descriptors (or add a
new helper pasteFiles) and synthesize a single clipboard event whose
clipboardData.files/items contains both files; then assert both 'first.txt' and
'second.pdf' are visible and that removeButtons (getByRole name 'Remove
attachment') has count 2. Ensure the new pasteFile/pasteFiles API is used by
this test so the app receives one multi-file paste rather than two single-file
pastes.
- Around line 230-275: The synthetic paste event bypasses browser insertion so
add an assertion (or better, use a real clipboard paste) to verify the textbox
actually receives "just plain text": in the test "pasting text (not a file)
should not create an attachment" after locating
textarea[name="inputchat-textarea"] (and using the chatInput variable), write
"just plain text" to the clipboard (e.g., navigator.clipboard.writeText or
Playwright's clipboard API), focus the textarea and simulate a paste keypress
(Ctrl/Meta+V), then assert the textarea's value or chatInput text equals "just
plain text" in addition to the existing attachment-absence and visibility checks
so the test fails if paste handling is broken.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6547140c-0356-4f29-a9b5-15d23e0c0a13
📒 Files selected for processing (3)
CHANGELOG.mdsrc/frontend/apps/e2e/__tests__/app-conversations/chat.spec.tssrc/frontend/apps/e2e/__tests__/app-conversations/file-paste.spec.ts
💤 Files with no reviewable changes (1)
- src/frontend/apps/e2e/tests/app-conversations/chat.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
Comment on lines
+187
to
+209
| test('pasting multiple different files shows all attachments', async ({ | ||
| page, | ||
| }) => { | ||
| await pasteFile(page, { | ||
| content: 'text content', | ||
| name: 'first.txt', | ||
| type: 'text/plain', | ||
| }); | ||
| await expect(page.getByText('first.txt')).toBeVisible({ timeout: 5000 }); | ||
|
|
||
| await pasteFile(page, { | ||
| content: '%PDF-1.4 content', | ||
| name: 'second.pdf', | ||
| type: 'application/pdf', | ||
| }); | ||
| await expect(page.getByText('second.pdf')).toBeVisible({ timeout: 5000 }); | ||
|
|
||
| // 2 attachments should be visible | ||
| await expect(page.getByText('first.txt')).toBeVisible(); | ||
| const removeButtons = page.getByRole('button', { | ||
| name: 'Remove attachment', | ||
| }); | ||
| await expect(removeButtons).toHaveCount(2); |
There was a problem hiding this comment.
This is still two single-file pastes, not one multi-file paste.
This only proves attachments accumulate across repeated paste events. A handler that ignores every file after clipboardData.files[0] would still pass here. Please dispatch both files in the same paste event, likely by extending pasteFile to accept an array.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/frontend/apps/e2e/__tests__/app-conversations/file-paste.spec.ts` around
lines 187 - 209, The test "pasting multiple different files shows all
attachments" currently simulates two separate paste events; change it to
dispatch both files in a single paste event by extending or overloading the
pasteFile helper (pasteFile) to accept an array of file descriptors (or add a
new helper pasteFiles) and synthesize a single clipboard event whose
clipboardData.files/items contains both files; then assert both 'first.txt' and
'second.pdf' are visible and that removeButtons (getByRole name 'Remove
attachment') has count 2. Ensure the new pasteFile/pasteFiles API is used by
this test so the app receives one multi-file paste rather than two single-file
pastes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Add a few tests for file pasting
Summary by CodeRabbit