Skip to content

fix(app): rewrite prompt-drop-file e2e to match attachment routing gate - #660

Merged
Astro-Han merged 2 commits into
devfrom
claude/i652-drop-debug
May 16, 2026
Merged

fix(app): rewrite prompt-drop-file e2e to match attachment routing gate#660
Astro-Han merged 2 commits into
devfrom
claude/i652-drop-debug

Conversation

@Astro-Han

@Astro-Han Astro-Han commented May 16, 2026

Copy link
Copy Markdown
Owner

Summary

Replace the failing packages/app/e2e/prompt/prompt-drop-file.spec.ts with two narrow tests that match the behavior the shared e2e backend can actually exercise.

Why

The legacy spec asserted that dropping any image always renders an attachment chip. After PR #565749412 (feat: route prompt composer attachments, 2026-04-22) introduced the model-gated routing in packages/app/src/components/prompt-input/attachment-routing.ts, that assertion is impossible to satisfy in e2e: the shared backend only exposes text-only opencode models (big-pickle and four other free models, all modalities.input: ["text"]), so routeBrowserFile rightly returns reject-image and the chip never appears. The spec has been silently failing on dev pushes ever since because it is not tagged @smoke and the e2e job downgrades failures to warnings, not blocking checks.

i652 was filed assuming a regression on dev. Investigation confirmed the drop wiring itself is healthy:

  • the document-level drop listener in attachments.ts fires
  • event.dataTransfer.files is populated
  • routeBrowserFile returns reject-image
  • warnImageUnsupported toast renders correctly with the "Choose model" CTA

The product behavior is correct. The test was outdated.

Related Issue

Closes #652.

Human Review Status

Pending. A human should make the final merge decision after reviewing the final diff and verification evidence.

Review Focus

  • Does the rejection-toast assertion read as the right product spec for text-only models? The active model in e2e is big-pickle; the toast title This model cannot read images and the Choose model button are taken verbatim from prompt.toast.imageUnsupported.* in packages/app/src/i18n/en.ts.
  • Is asserting on the chip via the filename text (drop.txt) acceptable as a stand-in for the image happy path? The image branch needs a vision-capable seed model in the e2e backend; tracked as a follow-up rather than bundled here.

Risk Notes

None. Test-only change, no product code touched. The drop wiring and the routing gate are unchanged.

How To Verify

Targeted prompt-drop-file.spec.ts (chromium):       2 passed / 4.5s
Targeted prompt-drop-file.spec.ts + uri spec:       3 passed / 4.8s
git diff --check:                                   no whitespace errors

Screenshots or Recordings

Not applicable. Test-only change; no UI surface changed.

Checklist

  • Human review status is stated above as pending, approved, or not required
  • I linked the related issue, or stated why there is no issue
  • This PR has type, primary area, and priority labels, or I requested maintainer labeling
  • I described the review focus and any meaningful risks
  • I listed the relevant verification steps and the key result for each
  • I did not introduce unrelated refactors, dependencies, generated files, or file changes beyond the stated scope
  • I manually checked visible UI or copy changes when needed, with screenshots or recordings
  • I considered macOS and Windows impact for platform, packaging, updater, signing, paths, shell, or permissions changes
  • I called out docs, release notes, dependencies, permissions, credentials, deletion behavior, generated content, or local file changes when relevant
  • I reviewed the final diff for unrelated changes and suspicious dependency changes
  • I am targeting dev, and my PR title and commit messages use Conventional Commits in English

Summary by CodeRabbit

Release Notes

  • New Features

    • File attachments: Users can now drop text files into prompts to create attachments. Each attachment appears as an interactive chip with hover effects and dedicated remove buttons for easy management and control.
  • Bug Fixes

    • Image handling: Dropping image files now displays an "unsupported images" toast notification, informing users about file type support constraints.

Review Change Stack

The legacy spec asserted that dropping any image always renders an
attachment chip. After the model-gated attachment routing landed, the
shared e2e backend (which only ships text-only opencode models) makes
that assertion impossible to satisfy: routeBrowserFile rightly returns
reject-image and the chip never appears. The spec has been silently
failing on dev pushes since then because it lacks @smoke and the e2e
job downgrades failures to warnings.

Replace the spec with two narrow tests that match what the test backend
can actually exercise:

- dropping an image with a text-only model surfaces the
  imageUnsupported toast and the "Choose model" CTA, with no chip
- dropping a text file routes through addDirect and renders a removable
  attachment chip via PromptImageAttachments' non-image fallback

The image happy path still needs a vision-capable seed model in the
e2e backend; tracking that as a follow-up rather than bundling it
here.

Closes #652
@coderabbitai

coderabbitai Bot commented May 16, 2026

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

The e2e test for file drop behavior is updated to reflect text-only backend constraints. Image drops now trigger an unsupported-images toast; text file drops create a removable attachment chip with interactive controls.

Changes

Drop-file e2e test updates for text-only backend

Layer / File(s) Summary
Drop-file test assertions: image rejection and text attachment
packages/app/e2e/prompt/prompt-drop-file.spec.ts
Test structure and comments document two drop-file pipeline behaviors under text-only constraints. Test logic validates that dropping an image triggers the unsupported-images toast, and dropping a text file creates a "drop.txt" attachment chip with a "Remove attachment" button that deletes the chip on click.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

  • Astro-Han/pawwork#295: Updates MIME type validation and toast emission for drag/drop file handling, which directly correlates with the drop-file e2e test changes that now expect unsupported-images toast behavior.

Poem

A test bunny hops through drop events with care, 🐰
Images bounce back with a toast in the air,
Text files descend on a chip, bright and neat,
Removable memories make the test complete! ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main change: rewriting the e2e test to align with the new attachment routing gate behavior introduced in a recent PR.
Description check ✅ Passed The description comprehensively covers all template sections including summary, rationale, related issue, review focus, risk assessment, verification steps, and a complete checklist.
Linked Issues check ✅ Passed The PR correctly addresses issue #652 by updating the failing e2e test to match the actual product behavior after the attachment routing gate was introduced, restoring test validity.
Out of Scope Changes check ✅ Passed All changes are within scope: only the e2e spec file is modified with no product code, dependencies, or unrelated refactors introduced.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/i652-drop-debug

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions Bot added the app Application behavior and product flows label May 16, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested priority: P3 (only low-risk paths changed (packages/app/e2e/prompt/prompt-drop-file.spec.ts)).

P1/P0 are reserved for maintainer confirmation. Please relabel manually if this is a release blocker, security issue, data-loss risk, or updater/runtime failure.

@Astro-Han Astro-Han added bug Something isn't working P2 Medium priority labels May 16, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request updates the E2E tests for the file-drop pipeline to account for backend constraints where only text-only models are available. It replaces the previous image attachment test with a scenario that verifies an unsupported model toast and adds a new test case for dropping and removing text file attachments. A review comment suggested improving the fidelity of the text file removal test by replacing a forced click with an explicit hover action, ensuring the test correctly simulates the UI behavior where the remove button only appears on hover.

Comment thread packages/app/e2e/prompt/prompt-drop-file.spec.ts Outdated
Per Gemini review: the remove button uses opacity-0 group-hover:opacity-100,
so { force: true } was bypassing actionability. Hover the chip first so the
button becomes interactive and let Playwright run the normal click path.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
packages/app/e2e/prompt/prompt-drop-file.spec.ts (1)

53-53: ⚡ Quick win

Scope the remove button query to the chip locator.

Currently the remove button is queried from the page level with .first(). Scoping it to the chip locator is more robust and makes the test intent explicit.

♻️ Scope the query to the chip
-  const remove = page.getByRole("button", { name: "Remove attachment" }).first()
+  const remove = chip.getByRole("button", { name: "Remove attachment" })
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/app/e2e/prompt/prompt-drop-file.spec.ts` at line 53, The remove
button is being selected from the whole page with page.getByRole(...).first(),
which is brittle; instead scope the query to the existing chip locator (e.g.,
use chip.getByRole("button", { name: "Remove attachment" }) ) so the test
explicitly targets the remove button inside that chip; update the variable
declaration for remove to use the chip locator (reference the existing chip
variable) and remove the .first() call.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@packages/app/e2e/prompt/prompt-drop-file.spec.ts`:
- Line 53: The remove button is being selected from the whole page with
page.getByRole(...).first(), which is brittle; instead scope the query to the
existing chip locator (e.g., use chip.getByRole("button", { name: "Remove
attachment" }) ) so the test explicitly targets the remove button inside that
chip; update the variable declaration for remove to use the chip locator
(reference the existing chip variable) and remove the .first() call.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: d7e4e82a-9431-4ec6-a9b3-6400f6326458

📥 Commits

Reviewing files that changed from the base of the PR and between 57cb802 and 4af467b.

📒 Files selected for processing (1)
  • packages/app/e2e/prompt/prompt-drop-file.spec.ts

@Astro-Han

Copy link
Copy Markdown
Owner Author

@coderabbitai re. nitpick on prompt-drop-file.spec.ts L53: skipping. page.getByText("drop.txt") resolves to the inner span (image-attachments.tsx:52), not the chip root div.relative.group (image-attachments.tsx:27). The remove button is a sibling of the name container, so chip.getByRole("button", ...) would not find it. There is only one chip in the test, so page-level + .first() is unambiguous — rewriting chip to grab a stable root selector adds noise without test-fidelity gain.

@coderabbitai

coderabbitai Bot commented May 16, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

@Astro-Han: Understood — the DOM layout makes the refactor actively wrong, not just noisy. Since page.getByText("drop.txt") lands on the inner span and the remove button is a sibling of that name container (not a descendant), scoping getByRole("button") off that locator would miss it entirely. Page-level + .first() is the correct approach here given the single-chip invariant of this test. Thanks for the clear walkthrough.


✏️ Learnings added
Learnt from: Astro-Han
URL: https://github.com/Astro-Han/pawwork/pull/660

Timestamp: 2026-05-16T01:07:01.667Z
Learning: In `packages/app/e2e/prompt/prompt-drop-file.spec.ts`, the attachment chip DOM (rendered by `image-attachments.tsx`) has this structure: the chip root is `div.relative.group` (line 27), the file name is an inner `span` (line 52), and the remove button is a sibling of the name container — not a descendant. Therefore, `page.getByText("drop.txt")` resolves to the inner span, and scoping `getByRole("button", ...)` off that locator will not find the remove button. Using page-level locators + `.first()` is correct when only one chip is present.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

@Astro-Han
Astro-Han merged commit 2185ca5 into dev May 16, 2026
26 checks passed
@Astro-Han
Astro-Han deleted the claude/i652-drop-debug branch May 16, 2026 01:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

app Application behavior and product flows bug Something isn't working P2 Medium priority

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Dragging an image into the prompt input does not attach it

1 participant