Remove attachment confirmation modal if any files match by ktuite · Pull Request #1705 · getodk/central-frontend

ktuite · 2026-07-13T21:11:01Z

This removes the confirmation step when you drag multiple attachments onto a form draft. As long as some of the filenames match the expected attachments, the matching files will be uploaded automatically. Only if none of the files match will there be a popup explaining how nothing could be uploaded.

Removed:
(dragging b.jpg and c.jpg vs dragging b.jpg, c.jpg, other.jpg)

Kept:
(dragging other.jpg, other2.jpg)

The popup is now simpler, and the table for rendering the attachments is also a bit simpler in that it wont highlight rows of pending attachments.

What has been done to verify that this works as intended?

I've tried it out and updated the tests. I had to make some changes to the tests

remove sections about testing the highlighting on the table for pending uploads
change the tests that automatically uploaded attachment data to include the POST requests and chain the requests together instead of using async/await

Why is this the best possible solution? Were any other approaches considered?

This is a pretty complex bit of code but I think I cut out the confirmation step in an acceptable way.

How does this change impact users? Describe intentional behavior changes from code updates. What are the regression risks?

Will help users not to forget to upload their attachments if dragging multiple attachments at once.

If the users have a mix of matching/non-matching filenames, they'll get less feedback about the non-matching filenames. But they still get feedback (via rows highlighted after upload) on which files did successfully upload.

Does this change require updates to user documentation? If so, please file an issue here and include the link below.

I don't think so.

changeset-bot · 2026-07-13T21:11:06Z

🦋 Changeset detected

Latest commit: 09f02ad

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@getodk/central-frontend	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

matthew-white

Leaving some questions/comments, but this looks great so far. It's a complex component for sure!

matthew-white · 2026-07-14T16:45:48Z

-      it('shows the correct labels', async () => {
-        const component = await load('/projects/1/forms/f/draft', {
-          root: false
-        });
-        await select(component, blankFiles(['a', 'b', 'd']));
-        const labels = component.findAllComponents(FormAttachmentRow)
-          .map(row => row.get('.label'));
-        labels[0].should.be.visible();
-        labels[0].text().should.equal('Replace');
-        labels[1].should.be.visible();
-        labels[1].text().should.equal('Upload');
-        labels[2].should.be.hidden();
-      });


Seeing these tests go away, I'm wondering, are we able to remove this label logic from the FormAttachmentRow component now? It'd be nice to simplify that if possible.

Similarly, is it possible to remove some of the highlighting logic from FormAttachmentRow now?

This sounded like a good idea at first, but the highlighting and labeling table logic is still used for

single file upload (like when you drag something onto a table and then the name mismatch modal shows up)

just dragging a set of files over the table

There might be a way to simplify it a little bit, but it would require untangling and understanding the rest of possible paths and I don't know if that's worth it. I think it's kinda nice UX here!

I may change my mind in the future if/when we have entity media attachments... like when there's a need to upload a bunch of files for an entity list and that wants to reuse some of this UI.

Ah that's a good point that some highlighting/labeling is still used. 👍

I think we should remove the following line. If plannedUploads is set, that now means that the request is sent immediately (it's in progress), and an overlay is shown over the table. So there's no reason to do the highlighting/labeling in that case. If we remove this line, then we can stop passing plannedUploads to FormAttachmentTable and FormAttachmentRow, simplifying the data flow.

central-frontend/apps/central/src/components/form-attachment/row.vue

Lines 113 to 114 in f6b6bbf

const targetedByDrop = this.plannedUploads

.some(({ attachment }) => attachment.name === this.attachment.name);

If plannedUploads is set, that now means that the request is sent immediately (it's in progress), and an overlay is shown over the table.

Or I guess another case is that the name mismatch modal is shown over the table (the request isn't in progress yet). But also in that case, I don't think we need the highlighting/labeling. In both cases, there's an overlay over the table.

matthew-white · 2026-07-14T16:49:54Z

I searched this file to see how unmatchedFiles was used, and I saw cancelUploads(). cancelUploads() is now only relevant in the no-matching-file case, right? If so, what do you think about removing this line from the method?

if (this.plannedUploads.length !== 0) this.plannedUploads = [];

Maybe we rename the method to cancelUnmatched() or clearUnmatched()?

Since it's a complex component, I think it's nice to make it clear how/where each data property can be modified.

cancelUploads() (now renamed to clearPendingFiles()) is used in a couple more places with the name mismatch modal (for a single file), the popup (when all files are mismatched), and something with resetting the drag enter, so it still needs the plannedUploads line.

OK, I see that the name mismatch modal in particular does need the plannedUploads line. 👍 I don't think the other cases need it, but one is enough.

Thinking more about it, I can't think of a case that now needs to clear both sets of files:

Name mismatch modal (for a single file): Needs to clear plannedUploads, but not unmatchedFiles (which should be empty).

The popup (when all files are mismatched): Needs to clear unmatchedFiles, but not plannedUploads (which should be empty).

Drag enter: Needs to clear unmatchedFiles, but not plannedUploads (which should be empty).

Whereas before, it really was necessary to clear both sets of files.

I think it'd make the data flow more explicit to use two different methods instead, one for each set of files. Then it'd be clearer when/how each set of files is mutated.

I'm also happy to leave it as-is, just wanted to throw out this idea.

matthew-white · 2026-07-14T16:56:08Z

      // With the changes to this.plannedUploads and this.unmatchedFiles, the
-      // popup will show in the next tick.
+      // popup will show in the next tick if there are no plannedUploads, only unmatched files.


What do you think about merging this comment with the one above? I feel like this one is slightly confusing now. I'm thinking of something like:

Automatically upload planned files without confirmation.
If there are no planned files, the popup will show in the next tick for unmatched files.

comments updated and improved

matthew-white · 2026-07-14T17:00:18Z

+      // Automatically upload planned files without confirmation
+      if (this.plannedUploads.length > 0)
+        this.uploadFiles();


In this case, what do you think about also clearing unmatchedFiles? We don't need to hold onto those files anymore, and clearing them may simplify the data flow.

unmatchedFiles are cleared as part of uploadFiles() so I don't think it's necessary to add a clearing step here. And when the files aren't uploaded, the unmatchedFiles are used in the next modal/popup.

unmatchedFiles are cleared as part of uploadFiles() so I don't think it's necessary to add a clearing step here.

Maybe not necessary, but I do feel like it'd improve the data flow. uploadFiles() doesn't use unmatchedFiles at all. So matchFilesToAttachments() ends up setting a data property, then calling another method that doesn't use the data property and just immediately resets it.

when the files aren't uploaded, the unmatchedFiles are used in the next modal/popup.

Yes, we definitely need it set in that case. One code pattern that might make everything more explicit is to use a different variable than this.unmatchedFiles. Idea: rather than setting this.plannedUploads and this.unmatchedFiles, the for-loop could set this.plannedUploads and a local variable (not a data property). Then only set the data property to that variable if that's needed for the popup. Something like:

this.plannedUploads = []; const unmatched = []; for (let i = 0; i < files.length; i += 1) { // ... } if (this.plannedUploads.length > 0) // Automatically upload planned files without confirmation. this.uploadFiles(); else // Setting this data property will show the popup in the next tick. this.unmatchedFiles = unmatched;

ktuite

Responding to comments

ktuite · 2026-07-16T23:56:54Z

cancelUploads() (now renamed to clearPendingFiles()) is used in a couple more places with the name mismatch modal (for a single file), the popup (when all files are mismatched), and something with resetting the drag enter, so it still needs the plannedUploads line.

ktuite · 2026-07-16T23:57:54Z

+      // Automatically upload planned files without confirmation
+      if (this.plannedUploads.length > 0)
+        this.uploadFiles();


unmatchedFiles are cleared as part of uploadFiles() so I don't think it's necessary to add a clearing step here. And when the files aren't uploaded, the unmatchedFiles are used in the next modal/popup.

ktuite · 2026-07-16T23:58:04Z

      // With the changes to this.plannedUploads and this.unmatchedFiles, the
-      // popup will show in the next tick.
+      // popup will show in the next tick if there are no plannedUploads, only unmatched files.


comments updated and improved

ktuite · 2026-07-17T00:02:55Z

-      it('shows the correct labels', async () => {
-        const component = await load('/projects/1/forms/f/draft', {
-          root: false
-        });
-        await select(component, blankFiles(['a', 'b', 'd']));
-        const labels = component.findAllComponents(FormAttachmentRow)
-          .map(row => row.get('.label'));
-        labels[0].should.be.visible();
-        labels[0].text().should.equal('Replace');
-        labels[1].should.be.visible();
-        labels[1].text().should.equal('Upload');
-        labels[2].should.be.hidden();
-      });


This sounded like a good idea at first, but the highlighting and labeling table logic is still used for

single file upload (like when you drag something onto a table and then the name mismatch modal shows up)

just dragging a set of files over the table

There might be a way to simplify it a little bit, but it would require untangling and understanding the rest of possible paths and I don't know if that's worth it. I think it's kinda nice UX here!

I may change my mind in the future if/when we have entity media attachments... like when there's a need to upload a bunch of files for an entity list and that wants to reuse some of this UI.

matthew-white · 2026-07-21T17:29:29Z

I like the new commit! I've left a few replies in the threads above. I think there are a couple more opportunities to improve the code and make this complex data flow clearer for our future selves.

…hlight a row

ktuite marked this pull request as ready for review July 13, 2026 21:29

ktuite requested a review from matthew-white July 13, 2026 21:29

matthew-white reviewed Jul 14, 2026

View reviewed changes

ktuite commented Jul 17, 2026

View reviewed changes

ktuite added 7 commits August 5, 2026 16:47

Remove attachment confirmation modal if any files match

be2d185

transifex string changes

50e2e98

patch changeset

3e1a8c7

Refine code

4002986

removed plannedUploads from row because its never visibly used to hig…

b4a5b07

…hlight a row

Split up clearPendingFiles to be more clear

ab97534

Update comment

09f02ad

ktuite force-pushed the ktuite/remove_attachment_confirmation branch from 9c8ed06 to 09f02ad Compare August 6, 2026 00:53

	const targetedByDrop = this.plannedUploads
	.some(({ attachment }) => attachment.name === this.attachment.name);

Uh oh!

Conversation

ktuite commented Jul 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What has been done to verify that this works as intended?

Why is this the best possible solution? Were any other approaches considered?

How does this change impact users? Describe intentional behavior changes from code updates. What are the regression risks?

Does this change require updates to user documentation? If so, please file an issue here and include the link below.

Uh oh!

changeset-bot Bot commented Jul 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

matthew-white left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthew-white Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ktuite left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthew-white commented Jul 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ktuite commented Jul 13, 2026 •

edited

Loading

changeset-bot Bot commented Jul 13, 2026 •

edited

Loading

matthew-white Jul 21, 2026 •

edited

Loading