Skip to content

feat: "download all" zip + scrollable list for envelope downloads - #88

Draft
reeseherber wants to merge 1 commit into
mainfrom
build-87
Draft

feat: "download all" zip + scrollable list for envelope downloads#88
reeseherber wants to merge 1 commit into
mainfrom
build-87

Conversation

@reeseherber

Copy link
Copy Markdown
Collaborator

Closes #87

From Freshservice ticket FS-161220 (reporter: Dana). Downloading signed documents from a multi-document envelope had three problems:

  1. No "download all" — each of an envelope's documents (one had 12) had to be downloaded individually.
  2. The download list was clipped — only ~8 of 12 documents were visible and the list could not be scrolled.
  3. From the envelope detail screen there was no obvious way to grab the documents.

Changes

  • Dependency-free ZIP writer (packages/lib/universal/zip.ts): a small store-mode ZIP encoder (CRC32 + central directory). PDFs are already compressed, so store mode is ideal and avoids adding a zip dependency. Validated against unzip -t and Python's zipfile.
  • Server routes: GET /api/files/envelope/:envelopeId/download-all/:version? (authenticated) and GET /api/files/token/:token/download-all/:version? (recipient) bundle every document in the envelope into one .zip named after the envelope. Reuses the existing access checks.
  • Download dialog: adds a Download all button (signed for completed envelopes, original otherwise) and makes the document list scrollable (max-h + overflow-y-auto) so every document is reachable regardless of count. The envelope title is threaded through for the archive filename.

The envelope detail screen's existing Download button opens this same (now fixed) dialog, addressing point 3.

apps/remix typecheck passes.

…ds (do-87)

The envelope download dialog listed each document with its own download
buttons but had no way to fetch them all at once, and the list had no
height constraint so long lists (e.g. 12 documents) overflowed the dialog
with no way to scroll to the clipped entries.

- Add a dependency-free, store-mode ZIP writer (packages/lib/universal/zip.ts)
  used server-side to bundle an envelope's documents into a single archive.
- Add authenticated and recipient-token "download-all" file routes that stream
  the bundled signed (or original) PDFs as a .zip named after the envelope.
- Add a "Download all" action to the envelope download dialog and make the
  document list scrollable (max-height + overflow) so every document is
  reachable regardless of count.

Closes #87

@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 introduces a feature to download all documents in an envelope as a single ZIP archive, implementing a custom, dependency-free ZIP writer utility alongside new server-side endpoints and UI integrations. The code review feedback highlights several critical improvements: parallelizing file fetching with Promise.all to boost performance, sanitizing document titles and filenames on both the client and server to prevent path traversal (Zip Slip) vulnerabilities, and optimizing database queries by using Prisma's findUnique instead of findFirst when querying by ID.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +195 to +220
for (const item of items) {
if (!item.documentData) {
continue;
}

const documentDataToUse =
version === 'signed' ? item.documentData.data : item.documentData.initialData;

const file = await getFileServerSide({
type: item.documentData.type,
data: documentDataToUse,
}).catch((error) => {
console.error(error);

return null;
});

if (!file) {
continue;
}

const baseTitle = item.title.replace(/\.pdf$/, '');
const name = dedupeFileName(`${baseTitle}${suffix}`, usedNames);

files.push({ name, data: file });
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

security-high high

Performance & Security Improvements

  1. Parallel File Fetching (Performance): Currently, getFileServerSide is awaited sequentially inside a for...of loop. For envelopes with many documents, this results in sequential network/I/O requests, significantly slowing down the ZIP generation. Using Promise.all allows fetching all files in parallel.
  2. Path Traversal / Zip Slip Prevention (Security): Document titles are not guaranteed to be safe filenames. If a title contains path traversal sequences (e.g., ../../etc/passwd), it could lead to a Zip Slip vulnerability when extracted. Sanitizing the title by replacing path separators (/ and \) with underscores ensures all files are safely extracted into the root of the ZIP archive.
  const filesResults = await Promise.all(
    items.map(async (item) => {
      if (!item.documentData) {
        return null;
      }

      const documentDataToUse =
        version === 'signed' ? item.documentData.data : item.documentData.initialData;

      const file = await getFileServerSide({
        type: item.documentData.type,
        data: documentDataToUse,
      }).catch((error) => {
        console.error(error);

        return null;
      });

      if (!file) {
        return null;
      }

      return { item, file };
    })
  );

  for (const result of filesResults) {
    if (!result) {
      continue;
    }

    const { item, file } = result;
    const baseTitle = item.title.replace(/[/\\]/g, '_').replace(/\.pdf$/, '');
    const name = dedupeFileName(`${baseTitle}${suffix}`, usedNames);

    files.push({ name, data: file });
  }

Comment on lines +228 to +229
const baseZipTitle = envelopeTitle.replace(/\.pdf$/, '') || 'documents';
const zipFilename = `${baseZipTitle}.zip`;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Sanitize ZIP Filename:
If the envelopeTitle contains path separators (e.g., / or \), it can corrupt the Content-Disposition header or cause browsers to fail the download. Sanitizing the title by replacing these characters with underscores ensures a safe and reliable download filename.

Suggested change
const baseZipTitle = envelopeTitle.replace(/\.pdf$/, '') || 'documents';
const zipFilename = `${baseZipTitle}.zip`;
const baseZipTitle = envelopeTitle.replace(/[/\\]/g, '_').replace(/\.pdf$/, '') || 'documents';
const zipFilename = `${baseZipTitle}.zip`;

Comment on lines +237 to +251
const envelope = await prisma.envelope.findFirst({
where: {
id: envelopeId,
},
include: {
envelopeItems: {
orderBy: {
order: 'asc',
},
include: {
documentData: true,
},
},
},
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Use findUnique instead of findFirst:
Since id is the primary key of the Envelope model, using findUnique is more idiomatic and allows Prisma to optimize the query execution plan better than findFirst.

Suggested change
const envelope = await prisma.envelope.findFirst({
where: {
id: envelopeId,
},
include: {
envelopeItems: {
orderBy: {
order: 'asc',
},
include: {
documentData: true,
},
},
},
});
const envelope = await prisma.envelope.findUnique({
where: {
id: envelopeId,
},
include: {
envelopeItems: {
orderBy: {
order: 'asc',
},
include: {
documentData: true,
},
},
},
});

Comment on lines +42 to +47
const baseTitle = (fileName ?? 'documents').replace(/\.zip$/, '');

downloadFile({
filename: `${baseTitle}.zip`,
data: blob,
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Sanitize Client-Side Filename:
Sanitize the fileName by replacing path separators (/ and \) with underscores to prevent issues when triggering the client-side file download.

Suggested change
const baseTitle = (fileName ?? 'documents').replace(/\.zip$/, '');
downloadFile({
filename: `${baseTitle}.zip`,
data: blob,
});
const baseTitle = (fileName ?? 'documents').replace(/[/\\]/g, '_').replace(/\.zip$/, '');
downloadFile({
filename: `${baseTitle}.zip`,
data: blob,
});

@reeseherber reeseherber added needs-validation PR awaiting AI validator review validating Validator polecat is reviewing needs-human-approval and removed needs-validation PR awaiting AI validator review validating Validator polecat is reviewing labels Jun 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Documenso

1 participant