Adds download/upload attachment archive UIs #1551

Spoffy · 2025-04-04T22:45:22Z

Context

The API supports:

Downloading all attachments from a Grist doc as an archive, for both internally and externally stored attachments.
Uploading a .tar archive of attachments, to re-add any missing attachments to the external store.

Neither of these functionalities is exposed via the UI currently.

Proposed solution

This PR adds:

An option to the copy menu for downloading attachment archives
An option to document settings to upload a .tar attachment archive

Screenshots will be added in the near future, this is currently in draft to allow previews.

Related issues

#1021

Has this been tested?

👍 yes, I added tests to the test suite
💭 no, because this PR is a draft and still needs work
🙅 no, because this is not relevant here
🙋 no, because I need help

Screenshots / Screencasts

github-actions · 2025-04-04T22:54:39Z

Deployed commit e4bf0be0b3c39ccb8841cbb8b8f0a4248d9f8449 as https://grist-gristlabs-grist-core-spoffy-archive-ui.fly.dev (until 2025-05-04T22:53:26.681Z)

paulfitz · 2025-04-07T14:05:13Z

I know this is still a draft but I gave it a try. Basic functionality works for me - some quick notes:

Maybe hide "upload missing attachments" if storage is internal?
My download was entitled "Untitled document-Attachments.zip" despite the document having a name set.
It would be good to remove the "cannot copy a document with external attachments" restriction at this time.

The download UI has clearly had some thought put into it, thanks for that!

github-actions · 2025-04-08T01:28:09Z

Deployed commit 1ca1afbafa1506c03112ce6cc983f32297e8c6aa as https://grist-gristlabs-grist-core-spoffy-archive-ui.fly.dev (until 2025-05-08T01:26:07.544Z)

# Conflicts: # app/client/ui/GristTooltips.ts

github-actions · 2025-04-12T13:47:33Z

Deployed commit 714c645b11c76c2f20a48e125328a17e502a425c as https://grist-gristlabs-grist-core-spoffy-archive-ui.fly.dev (until 2025-05-12T13:46:21.053Z)

github-actions · 2025-04-15T13:00:42Z

Deployed commit a61d9d28998f7550290f9e677239245a5d84e094 as https://grist-gristlabs-grist-core-spoffy-archive-ui.fly.dev (until 2025-05-15T12:59:16.814Z)

github-actions · 2025-04-15T13:22:04Z

Deployed commit b2e41c8f7bd7e02b6b7a8d4aa4a259dc0d1a3b89 as https://grist-gristlabs-grist-core-spoffy-archive-ui.fly.dev (until 2025-05-15T13:20:43.504Z)

github-actions · 2025-04-29T13:19:28Z

Deployed commit 414c922ca10abf3b621b8ca2cacf419ef52576a3 as https://grist-gristlabs-grist-core-spoffy-archive-ui.fly.dev (until 2025-05-29T13:17:59.283Z)

github-actions · 2025-04-29T17:40:46Z

Deployed commit b3062978bc5711778978aa76d36bcd9987f46d1f as https://grist-gristlabs-grist-core-spoffy-archive-ui.fly.dev (until 2025-05-29T17:38:51.014Z)

github-actions · 2025-04-29T17:42:33Z

Deployed commit e15f0aed7f3a1b3dd46d49f620bc6a7c3826e3fe as https://grist-gristlabs-grist-core-spoffy-archive-ui.fly.dev (until 2025-05-29T17:41:01.180Z)

github-actions · 2025-04-29T21:03:41Z

Deployed commit 73142445e65217a7ae3e514b75abd2a748562a68 as https://grist-gristlabs-grist-core-spoffy-archive-ui.fly.dev (until 2025-05-29T21:01:52.495Z)

berhalak · 2025-05-05T13:33:33Z

test/nbrowser/AttachmentsTransfer.ts

+      assert.equal(downloadUrl.pathname, idealUrl.pathname, "wrong download link called");
+      assert.equal(downloadUrl.search, idealUrl.search, "wrong search parameters in url");
+      // Ensures the page isn't modified / navigated away from by the link, as subsequent tests will fail.
+      await driver.find('.test-external-attachments-info a').click();


Why are you clicking this link here?

There was a bug with the link, where it accidentally changes the current page instead of downloading the .tar file.

This clicks it, because if that bug re-occurred then all tests after this one would break.
Although actually, this might be better handled either as a separate test or with additional checks that the current window's URL hasn't changed.

berhalak · 2025-05-05T13:36:20Z

test/nbrowser/AttachmentsTransfer.ts

+          response.data,
+          fs.createWriteStream(path.join(tmpDownloadsFolder, fileName))
+        );
+        assert.include(await fse.readdir(tmpDownloadsFolder), 'attachments.tar', "attachments file wasn't downloaded");


Can you test first that this file is not there, before downloading it?

Yup, can add that!

berhalak · 2025-05-05T19:21:57Z

app/client/ui/DocumentSettings.ts

+          if (!file) {
+            return;
+          }
+          isUploadingObs.set(true);


In the presence of the comment above, I think it is better to test if the owner is not disposed after promise returns? (Here and below).

I'm not sure what you mean, can you describe the change in more detail?

Are you suggesting we check if the owner is disposed before setting the observable? If so, for my benefit learning GrainJS, what's the reason to do that?

You are doing an async operation. By the time the promise is resolved, someone could move to another page, destroying all observables you are setting.

I've pushed some guards for this. Is this something that Observables should be handling internally?

It feels like once disposed, they should either error or do nothing if set is called on them.

Let me know if the additional checks are sufficient!

berhalak · 2025-05-05T19:23:14Z

app/client/ui/GristTooltips.ts

+        "If you want to re-import this document, " +
+        "a separate attachments archive will be needed to restore attachments. "
+      ),
+      dom('div', cssLink({href: "", target: "_blank"}, t('Learn more.'))),


Can you add a stub in common urls?

I'll go a step further and delete this - this was actually dead code, I just realised. Will double check the rest of the PR too.

app/client/ui/MakeCopyMenu.ts

berhalak · 2025-05-05T19:37:43Z

app/client/ui/MakeCopyMenu.ts

+
+// Prevents the div from expanding the parent and makes it only use available space instead.
+const cssEagerWrap = styled('div', `
+  min-width: 100%;


Very hacky, didn't know this trick. Wdyt about modern alternative ?
contain: inline-size;

Oooh, this is nice. Seems to do the same job in practice. Much cleaner than the hack above too.

Weird that this didn't come up when I was searching for options here!

Pushed this fix.

berhalak · 2025-05-05T19:38:53Z

app/client/ui/ShareMenu.ts

+      () => downloadAttachmentsModal(doc, pageModel),
+      menuIcon('Download'),
+      t('Download attachments...'),
+      testId('tb-share-option'),


Should we call this testId in some other way?

It's convention for everything in this list to have the same test id right now (tb-share-option).

I'm happy to fix that - but it needs to be a separate PR or two, as SaaS uses these test ids too.

app/common/UserAPI.ts

berhalak · 2025-05-05T19:40:41Z

app/common/UserAPI.ts

@@ -1200,7 +1220,7 @@ export class DocAPIImpl extends BaseAPI implements DocAPI {

  public async uploadAttachment(value: string | Blob, filename?: string): Promise<number> {
    const formData = this.newFormData();
-    formData.append('upload', value as Blob, filename);
+    formData.append('upload', value, filename);


Hmm, why we don't need it anymore? (Maybe it was needed in saas build).

FormData objects natively accept string | Blob in .append. I just looked through the Git history, and couldn't see any obvious reason why this was added. Maybe the types accepted by FormData.append changed?

Seems fine now either way!

I will check in saas's version of typescript. I remember I was the one added it as the types were complaining.

Great, let me know if it complains :)

berhalak · 2025-05-05T19:41:28Z

app/common/UserAPI.ts

+      data: formData,
+      // On browser, it is important not to set Content-Type so that the browser takes care
+      // of setting HTTP headers appropriately.  Outside browser, requestAxios has logic
+      // for setting the HTTP headers.


Do you have any links to reference?

Did some digging, and found a link which describes why this is important.
I updated the comment to this:

// On the browser, Content-Type shouldn't be set as it prevents the browser from setting
// Content-Type with the correct boundary expression to delimit form fields.
// https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest_API/Using_FormData_Objects#sending_files_using_a_formdata_object
// Therefore we omit Content-Type, and allow Axios to handle it as it sees fit - which works
// correctly in the browser and in Node.

berhalak · 2025-05-05T19:46:29Z

app/client/lib/uploads.ts

+ * @param {ProgressCB} onProgress - Called periodically during the upload
+ * @returns {Promise<any>} - Parsed JSON from the endpoint. Uses `any` as no validation is performed.
+ */
+export async function uploadFormData(


Is this used anywhere else?

Only in 'uploadFiles', which it was extracted from.

It was going to be used elsewhere, but I guess I went a different route.

I'm happy to leave this in though if you are, as it splits up a long function (uploadFiles), making it more readable.

Fine with leaving, maybe just remove the export, to detect dead code more easily in the future.

# Conflicts: # app/client/ui/MakeCopyMenu.ts

github-actions · 2025-05-05T23:25:30Z

Deployed commit ed65e9a2a7deb7c67df093ed745029989b9ee33d as https://grist-gristlabs-grist-core-spoffy-archive-ui.fly.dev (until 2025-06-04T23:24:10.420Z)

berhalak

I have a situation like in a screen below, so there are options to download attachemnts, there is a text that attachments are external, but document settings says opposite.

Also, I don't have any attachments whatsoever, should we still show this ui?

Spoffy · 2025-05-06T13:58:47Z

Good catch - looks like I didn't set the condition correctly for documents with no attachments. I've fixed that now!

github-actions · 2025-05-06T14:06:16Z

Deployed commit f48dd7cc390cdd41ffc8692a09f82ff022f01d5e as https://grist-gristlabs-grist-core-spoffy-archive-ui.fly.dev (until 2025-06-05T14:05:01.143Z)

github-actions · 2025-05-07T00:24:49Z

Deployed commit 0407dc4c0fed5c9a74c249afde3d50fc645ff088 as https://grist-gristlabs-grist-core-spoffy-archive-ui.fly.dev (until 2025-06-06T00:23:17.053Z)

berhalak · 2025-05-07T12:19:50Z

test/nbrowser/AttachmentsTransfer.ts

@@ -335,5 +426,24 @@ async function waitForNotPresent(fn: () => WebElementPromise) {

 const attachmentSection = () => driver.find('.test-admin-panel-item-preferredStorage');

+/*


Can be removed

Spoffy added preview Launch preview deployment of this PR -UX/UI labels Apr 4, 2025

github-project-automation bot added this to Grist UI/UX Apr 4, 2025

Spoffy added 15 commits April 12, 2025 14:13

Improves error handling for archives

ab5cf00

Cleans zip archive handling and removes async requirement

44ec8f1

Updates archive tests

22c61a4

Tidies imports

b6787f4

Adds missing res.end()

d471af8

Adds download button for archives

310f0e4

Extracts uploadFormData from uploadFiles

db00fb6

Adds archive upload UI

a11a2f8

# Conflicts: # app/client/ui/GristTooltips.ts

Adds archive upload UI

ece3119

Fixes compile-time error caused by import

e2c7a02

Improves attachment upload notification phrasing

970002f

Updates attachments downloads to new design

006a532

Updates copy menu designs + tests

1cbead6

Iterates on attachment download modal

1857f7d

WIP

714c645

Spoffy force-pushed the spoffy/archive-ui branch from 1ca1afb to 714c645 Compare April 12, 2025 13:40

Spoffy added 2 commits April 15, 2025 13:49

Adds upload tests

06799d9

Adds test for re-upload warning

a61d9d2

Spoffy marked this pull request as ready for review April 15, 2025 12:56

Fixes linter failure

b2e41c8

Spoffy marked this pull request as draft April 15, 2025 20:25

berhalak self-requested a review April 29, 2025 13:41

Spoffy added 2 commits April 29, 2025 18:33

Prevents download link changing page

b306297

Changes file format select style

e15f0ae

Improves error case on chrome

7314244

paulfitz added the Attachments label May 2, 2025

berhalak reviewed May 5, 2025

View reviewed changes

Spoffy mentioned this pull request May 5, 2025

Documents exporting and restoring external attachments gristlabs/grist-help#505

Open

berhalak reviewed May 5, 2025

View reviewed changes

Spoffy added 7 commits May 5, 2025 23:28

Improves test with extra assert

106f686

Cleans import ordering

808745f

Removes unused tooltip

45b004e

Fixes indentation

11e5397

Clarifies comment on archive upload method

46691bc

Changes to a less-hacky method of eager wrapping

d615998

Merge branch 'main' into spoffy/archive-ui

ed65e9a

# Conflicts: # app/client/ui/MakeCopyMenu.ts

berhalak reviewed May 6, 2025

View reviewed changes

Fix condition for .tar download prompt showing

f48dd7c

Spoffy added 2 commits May 7, 2025 01:12

Removes export from uploadFormData

697dc3a

Guard against setting a possibly disposed observable

0407dc4

Spoffy mentioned this pull request May 7, 2025

Re-enables copying for external attachment docs #1603

Draft

1 task

berhalak reviewed May 7, 2025

View reviewed changes

berhalak approved these changes May 7, 2025

View reviewed changes

		@@ -335,5 +426,24 @@ async function waitForNotPresent(fn: () => WebElementPromise) {

		const attachmentSection = () => driver.find('.test-admin-panel-item-preferredStorage');

		/*

Adds download/upload attachment archive UIs #1551

Are you sure you want to change the base?

Adds download/upload attachment archive UIs #1551

Conversation

Spoffy commented Apr 4, 2025 • edited Loading

Context

Proposed solution

Related issues

Has this been tested?

Screenshots / Screencasts

github-actions bot commented Apr 4, 2025

paulfitz commented Apr 7, 2025 • edited Loading

github-actions bot commented Apr 8, 2025

github-actions bot commented Apr 12, 2025

github-actions bot commented Apr 15, 2025

github-actions bot commented Apr 15, 2025

github-actions bot commented Apr 29, 2025

github-actions bot commented Apr 29, 2025

github-actions bot commented Apr 29, 2025

github-actions bot commented Apr 29, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Spoffy May 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Spoffy May 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Spoffy May 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 5, 2025

berhalak left a comment

Choose a reason for hiding this comment

Spoffy commented May 6, 2025

github-actions bot commented May 6, 2025

github-actions bot commented May 7, 2025

Choose a reason for hiding this comment

Spoffy commented Apr 4, 2025 •

edited

Loading

paulfitz commented Apr 7, 2025 •

edited

Loading

Spoffy May 5, 2025 •

edited

Loading

Spoffy May 5, 2025 •

edited

Loading

Spoffy May 5, 2025 •

edited

Loading