fix(deploy): skip deploy creation when the source zip is empty#8339
fix(deploy): skip deploy creation when the source zip is empty#8339davbree wants to merge 1 commit into
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR refactors source-zip handling for deploys. Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/commands/deploy/deploy.ts (1)
1425-1447: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMove the source-zip upload under the cancel-on-error try at
src/commands/deploy/deploy.ts:1425-1447
api.createSiteDeploy()creates the deploy beforeuploadSourceZipFile()runs, but this upload is still outside thetrythat callscancelDeploy(). If the upload throws, the new deploy is left open. Move the upload/cleanup block into that same cancelingtry, like the--no-buildpath.🤖 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 `@src/commands/deploy/deploy.ts` around lines 1425 - 1447, The source-zip upload and cleanup logic in the deploy flow is still outside the cancel-on-error try, so a failure in uploadSourceZipFile can leave the deploy created by api.createSiteDeploy open. Move the sourceZipPath handling block into the same try/catch that calls cancelDeploy, mirroring the --no-build path, and keep using deployMetadata, deployId, and sourceZipFileName so any upload or cleanup failure triggers deploy cancellation.
🧹 Nitpick comments (3)
tests/unit/utils/deploy/upload-source-zip.test.ts (1)
171-187: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider asserting
statusCb/warnon upload failure too.The
createSourceZipfailure test (Lines 106-122) asserts bothstatusCbandwarnare invoked on error, but this analogousuploadSourceZipFilefailure test only checks the thrown error andunlink. PeruploadSourceZipFile's contract, it also emits astatusCberror event and callswarnbefore rethrowing — worth covering for parity and to guard against future regressions in that error-reporting path.🤖 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 `@tests/unit/utils/deploy/upload-source-zip.test.ts` around lines 171 - 187, The upload failure test for uploadSourceZipFile only verifies the thrown error and cleanup, but it should also cover the error-reporting contract. Update the test to assert that the statusCb passed into uploadSourceZipFile receives the expected error status and that warn is called when the fetch upload fails, similar to the createSourceZip failure test. Use the uploadSourceZipFile function and its existing error path to locate where to exercise these callbacks while keeping the unlink assertion.src/utils/deploy/upload-source-zip.ts (1)
75-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop comments that restate what the code does.
// Check for Windows,// Create exclusion list for zip command, and// Use system zip command to create the archivedescribe the mechanics rather than intent; the code is already self-explanatory here.As per coding guidelines: "Never write comments on what the code does; make the code clean and self-explanatory instead."
Also applies to: 88-91
🤖 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 `@src/utils/deploy/upload-source-zip.ts` at line 75, Remove the comments in uploadSourceZip that merely restate the code’s mechanics, including the Windows check and the zip/exclusion list setup, and rely on the existing code structure in upload-source-zip.ts to remain self-explanatory. Keep only comments that add intent or non-obvious context; if needed, clarify the relevant logic around uploadSourceZip and any helper blocks rather than describing what each statement does.Source: Coding guidelines
src/commands/deploy/deploy.ts (1)
627-627: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winComment restates the code.
// We created a zip but the deploy didn't provide an upload URL — clean it up.describes what the following line does. Prefer self-explanatory code without the narration.As per coding guidelines: "Never write comments on what the code does; make the code clean and self-explanatory instead."
Also applies to: 1445-1446
🤖 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 `@src/commands/deploy/deploy.ts` at line 627, Remove the redundant narration comment in the deploy flow and any similar comments around the upload-URL cleanup path; the code in deploy.ts should be self-explanatory without restating what the cleanup logic does. Locate the cleanup branch in the deploy command (near the zip/upload URL handling) and delete the comment(s) so the intent is conveyed by the surrounding code and variable names instead.Source: Coding guidelines
🤖 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.
Inline comments:
In `@src/utils/deploy/upload-source-zip.ts`:
- Around line 154-164: cleanupSourceZip currently only deletes the source.zip
file, but createSourceZip allocates a temporary mkdtemp directory that also
needs removal. Update cleanupSourceZip to remove the parent temp directory (and
its contents if needed) instead of just unlinking zipPath, and make sure every
early-exit/error path that follows createSourceZip also invokes this cleanup so
no empty temp dirs are left behind.
---
Outside diff comments:
In `@src/commands/deploy/deploy.ts`:
- Around line 1425-1447: The source-zip upload and cleanup logic in the deploy
flow is still outside the cancel-on-error try, so a failure in
uploadSourceZipFile can leave the deploy created by api.createSiteDeploy open.
Move the sourceZipPath handling block into the same try/catch that calls
cancelDeploy, mirroring the --no-build path, and keep using deployMetadata,
deployId, and sourceZipFileName so any upload or cleanup failure triggers deploy
cancellation.
---
Nitpick comments:
In `@src/commands/deploy/deploy.ts`:
- Line 627: Remove the redundant narration comment in the deploy flow and any
similar comments around the upload-URL cleanup path; the code in deploy.ts
should be self-explanatory without restating what the cleanup logic does. Locate
the cleanup branch in the deploy command (near the zip/upload URL handling) and
delete the comment(s) so the intent is conveyed by the surrounding code and
variable names instead.
In `@src/utils/deploy/upload-source-zip.ts`:
- Line 75: Remove the comments in uploadSourceZip that merely restate the code’s
mechanics, including the Windows check and the zip/exclusion list setup, and
rely on the existing code structure in upload-source-zip.ts to remain
self-explanatory. Keep only comments that add intent or non-obvious context; if
needed, clarify the relevant logic around uploadSourceZip and any helper blocks
rather than describing what each statement does.
In `@tests/unit/utils/deploy/upload-source-zip.test.ts`:
- Around line 171-187: The upload failure test for uploadSourceZipFile only
verifies the thrown error and cleanup, but it should also cover the
error-reporting contract. Update the test to assert that the statusCb passed
into uploadSourceZipFile receives the expected error status and that warn is
called when the fetch upload fails, similar to the createSourceZip failure test.
Use the uploadSourceZipFile function and its existing error path to locate where
to exercise these callbacks while keeping the unlink assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4a65f7f1-5c6f-4f4e-b7b2-0d3119be881b
📒 Files selected for processing (3)
src/commands/deploy/deploy.tssrc/utils/deploy/upload-source-zip.tstests/unit/utils/deploy/upload-source-zip.test.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
netlify/blueprints(manual)
davbree
force-pushed
the
empty-source-zip
branch
2 times, most recently
from
25b7a9c to
d8094f0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/commands/deploy/deploy.ts (2)
1393-1431: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSame orphaned-zip pattern as
runDeploy(Lines 609-637).See the comment on Lines 609-637 for the full analysis —
sourceZipPathbuilt here at Line 1393 is only cleaned up if it reachesuploadSourceZipat Line 1425; ifapi.createSiteDeploythrows, or the response lacks upload metadata, the zip is left on disk with no cleanup path.🤖 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 `@src/commands/deploy/deploy.ts` around lines 1393 - 1431, The deploy flow leaves the temporary source zip orphaned when `api.createSiteDeploy` fails or returns no upload metadata, because `sourceZipPath` is only used in the `uploadSourceZip` branch. Update the `runDeploy`/deploy logic around `buildSourceZipOrExit`, `createSiteDeploy`, and `uploadSourceZip` to ensure the zip is always cleaned up, typically by tracking the generated path and removing it in a `finally` block or equivalent cleanup path even when deployment creation or upload setup fails.
609-637: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winClean up the temp source-zip directory when upload is skipped or fails.
uploadSourceZiponly unlinks the.zip, so ifcreateSiteDeploythrows or returns nosource_zip_upload_url/source_zip_filename, themkdtempdirectory fromcreateSourceZipis left behind. Add afinallycleanup that removes the whole temp dir here; the same pattern appears later in this command.🤖 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 `@src/commands/deploy/deploy.ts` around lines 609 - 637, The deploy flow in deploy() leaves the temporary source-zip directory behind when createSiteDeploy fails or when source_zip_upload_url/source_zip_filename is missing, because only the zip file is cleaned up by uploadSourceZip. Add a finally-style cleanup around the createSourceZip path here so the entire mkdtemp directory is removed regardless of upload success or skip, and follow the same cleanup pattern used later in this command. Reference the existing createSourceZip and uploadSourceZip flow to place the cleanup correctly.
♻️ Duplicate comments (1)
src/utils/deploy/upload-source-zip.ts (1)
80-80: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winOnly the zip file is cleaned up, not its parent temp directory.
createSourceZipallocates a freshmkdtempdirectory (Line 80:join(temporaryDirectory(), 'source.zip')), but the cleanup inuploadSourceZip'sfinallyblock only unlinkszipPath(Lines 191-193), leaving the now-empty parent directory behind on every successful/failed upload. It also isn't cleaned whencreateSourceZipthrowsEmptySourceZipError(the directory is created before thezipcommand runs). This accumulates orphaned temp directories over repeated deploys.🧹 Proposed fix
-import { readFile, unlink } from 'node:fs/promises' -import { join } from 'node:path' +import { readFile, rm } from 'node:fs/promises' +import { dirname, join } from 'node:path'} finally { - // Best-effort cleanup of the temporary zip. - try { - await unlink(zipPath) - } catch { - // Ignore cleanup errors - } + // Best-effort cleanup of the temporary zip and its directory. + try { + await rm(dirname(zipPath), { recursive: true, force: true }) + } catch { + // Ignore cleanup errors + } }Also applies to: 158-196
🤖 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 `@src/utils/deploy/upload-source-zip.ts` at line 80, The cleanup in uploadSourceZip only removes zipPath and leaves the mkdtemp parent directory behind, including when createSourceZip fails with EmptySourceZipError. Update uploadSourceZip and createSourceZip so the temporary directory created via temporaryDirectory() is always removed in the finally path, not just the file; use the existing zipPath/temporary directory handling in uploadSourceZip to clean up both the archive and its parent dir after upload or on error.
🤖 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.
Inline comments:
In `@src/utils/deploy/upload-source-zip.ts`:
- Around line 59-66: The JSDoc for the source zip helper is out of sync with the
exported API because it references uploadSourceZipFile and cleanupSourceZip,
which do not exist in this module. Update the comment on uploadSourceZip to
refer to the correct helper name or rephrase it so it says the caller uploads or
removes the temporary file itself, keeping the documentation aligned with the
actual exports.
---
Outside diff comments:
In `@src/commands/deploy/deploy.ts`:
- Around line 1393-1431: The deploy flow leaves the temporary source zip
orphaned when `api.createSiteDeploy` fails or returns no upload metadata,
because `sourceZipPath` is only used in the `uploadSourceZip` branch. Update the
`runDeploy`/deploy logic around `buildSourceZipOrExit`, `createSiteDeploy`, and
`uploadSourceZip` to ensure the zip is always cleaned up, typically by tracking
the generated path and removing it in a `finally` block or equivalent cleanup
path even when deployment creation or upload setup fails.
- Around line 609-637: The deploy flow in deploy() leaves the temporary
source-zip directory behind when createSiteDeploy fails or when
source_zip_upload_url/source_zip_filename is missing, because only the zip file
is cleaned up by uploadSourceZip. Add a finally-style cleanup around the
createSourceZip path here so the entire mkdtemp directory is removed regardless
of upload success or skip, and follow the same cleanup pattern used later in
this command. Reference the existing createSourceZip and uploadSourceZip flow to
place the cleanup correctly.
---
Duplicate comments:
In `@src/utils/deploy/upload-source-zip.ts`:
- Line 80: The cleanup in uploadSourceZip only removes zipPath and leaves the
mkdtemp parent directory behind, including when createSourceZip fails with
EmptySourceZipError. Update uploadSourceZip and createSourceZip so the temporary
directory created via temporaryDirectory() is always removed in the finally
path, not just the file; use the existing zipPath/temporary directory handling
in uploadSourceZip to clean up both the archive and its parent dir after upload
or on error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7326f10f-c159-4699-a1eb-5ab72cba90d6
📒 Files selected for processing (3)
src/commands/deploy/deploy.tssrc/utils/deploy/upload-source-zip.tstests/unit/utils/deploy/upload-source-zip.test.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
netlify/blueprints(manual)
When deploying with `--upload-source-zip`, the deploy record was created
before the source zip was built. If the zip came back empty ("zip error:
Nothing to do!" — everything matched an ignore pattern or the working tree
was empty), the deploy was left dangling in the UI (the build path didn't
even cancel it) and the command failed with a generic error.
Now the source zip is built *first*; the deploy is only created when it is
non-empty. When empty, the command exits with code 12 (mirroring zip's own
"nothing to do" exit code) so callers can detect this non-failure outcome.
`uploadSourceZip` is split into `createSourceZip` / `uploadSourceZipFile`
(+ `cleanupSourceZip`), and a typed `EmptySourceZipError` is introduced.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/unit/utils/deploy/upload-source-zip.test.ts (1)
125-142: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWindows test doesn't verify
warn/cleanup, unlike the sibling failure test.The Windows-platform failure goes through the same general-failure branch as
'reports and rethrows other zip failures'(callswarnand cleans up the temp dir), but this test only asserts the thrown message andstatusCb. Given the temp-dir cleanup was previously a flagged bug, assertingfs.rm(andwarn) here would guard against regressions on this specific early-exit path too.✅ Suggested additional assertions
test('throws on Windows platform', async () => { const mockOs = await import('os') vi.mocked(mockOs.platform).mockReturnValue('win32') const { createSourceZip } = await import('../../../../src/utils/deploy/upload-source-zip.js') + const mockCommandHelpers = await import('../../../../src/utils/command-helpers.js') + const mockFs = await import('fs/promises') const statusCb = vi.fn() await expect(createSourceZip({ sourceDir: '/test/source', statusCb })).rejects.toThrow( 'Source zip upload is not supported on Windows', ) expect(statusCb).toHaveBeenCalledWith( expect.objectContaining({ type: 'source-zip-upload', phase: 'error', msg: 'Failed to create source zip: Source zip upload is not supported on Windows', }), ) + expect(mockCommandHelpers.warn).toHaveBeenCalledWith( + 'Failed to create source zip: Source zip upload is not supported on Windows', + ) + expect(mockFs.rm).toHaveBeenCalledWith('/tmp/test-temp-dir', { recursive: true, force: true }) })As per coding guidelines, "Unit tests should test individual modules and utilities in
tests/unit/directory" - this strengthens coverage of the module's cleanup contract.🤖 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 `@tests/unit/utils/deploy/upload-source-zip.test.ts` around lines 125 - 142, The Windows-path test for createSourceZip currently only checks the thrown error and statusCb, but it should also cover the same cleanup/warning behavior as the existing failure case. Update the createSourceZip Windows test to assert the warn path is invoked and the temporary directory cleanup via fs.rm happens, using the same mocked failure flow and symbols already present in upload-source-zip.test.ts. This will verify the early-exit branch still performs the expected cleanup contract on win32.Source: Path instructions
🤖 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.
Inline comments:
In `@src/utils/deploy/upload-source-zip.ts`:
- Around line 78-93: Remove the redundant narrative comments in uploadSourceZip
that merely restate the next statements; keep the logic in platform() check,
DEFAULT_IGNORE_PATTERNS handling, and the zip command try block unchanged, and
rely on the code itself for clarity.
- Around line 59-132: The deploy flow currently leaves the temporary source zip
directory behind when `createSourceZip` succeeds but the `uploadSourceZip`
branch is skipped because the response lacks both `source_zip_upload_url` and
`source_zip_filename`, or an error occurs before upload. Add a caller-side
cleanup path in the deploy logic that owns `sourceZipPath` so the temp directory
is removed whenever no upload happens, while keeping `uploadSourceZip`
responsible for the successful upload case.
---
Nitpick comments:
In `@tests/unit/utils/deploy/upload-source-zip.test.ts`:
- Around line 125-142: The Windows-path test for createSourceZip currently only
checks the thrown error and statusCb, but it should also cover the same
cleanup/warning behavior as the existing failure case. Update the
createSourceZip Windows test to assert the warn path is invoked and the
temporary directory cleanup via fs.rm happens, using the same mocked failure
flow and symbols already present in upload-source-zip.test.ts. This will verify
the early-exit branch still performs the expected cleanup contract on win32.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: db1eb6f1-1659-4e93-9bb6-f3e97cff9bbd
📒 Files selected for processing (3)
src/commands/deploy/deploy.tssrc/utils/deploy/upload-source-zip.tstests/unit/utils/deploy/upload-source-zip.test.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
netlify/blueprints(manual)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/commands/deploy/deploy.ts
| /** | ||
| * Creates the source zip in a temporary directory and returns its path. The | ||
| * caller uploads it via `uploadSourceZip`, which removes the temporary directory | ||
| * afterwards. On any failure (including an empty archive) the directory is | ||
| * removed here before throwing. | ||
| * | ||
| * Throws `EmptySourceZipError` when the archive would be empty, so the caller | ||
| * can avoid creating a deploy for a source that has nothing to upload. | ||
| */ | ||
| export const createSourceZip = async ({ | ||
| sourceDir, | ||
| filename, | ||
| statusCb, | ||
| statusCb = () => {}, | ||
| }: { | ||
| sourceDir: string | ||
| filename: string | ||
| statusCb: (status: DeployEvent) => void | ||
| }) => { | ||
| // Check for Windows - this feature is not supported on Windows | ||
| if (platform() === 'win32') { | ||
| throw new Error('Source zip upload is not supported on Windows') | ||
| } | ||
| statusCb?: (status: DeployEvent) => void | ||
| }): Promise<string> => { | ||
| const zipPath = join(temporaryDirectory(), 'source.zip') | ||
|
|
||
| const tmpDir = temporaryDirectory() | ||
| const zipPath = join(tmpDir, filename) | ||
| try { | ||
| // Check for Windows - this feature is not supported on Windows | ||
| if (platform() === 'win32') { | ||
| throw new Error('Source zip upload is not supported on Windows') | ||
| } | ||
|
|
||
| // Ensure the directory for the zip file exists | ||
| // The filename from the API includes a subdirectory path (e.g., 'workspace-snapshots/source-xxx.zip') | ||
| // While temporaryDirectory() creates a new empty directory, the subdirectory within it doesn't exist | ||
| // so we need to create it before the zip command can write the file | ||
| await mkdir(dirname(zipPath), { recursive: true }) | ||
| statusCb({ | ||
| type: 'source-zip-upload', | ||
| msg: `Creating source zip...`, | ||
| phase: 'start', | ||
| }) | ||
|
|
||
| statusCb({ | ||
| type: 'source-zip-upload', | ||
| msg: `Creating source zip...`, | ||
| phase: 'start', | ||
| }) | ||
| // Create exclusion list for zip command | ||
| const excludeArgs = DEFAULT_IGNORE_PATTERNS.flatMap((pattern) => ['-x', pattern]) | ||
|
|
||
| // Create exclusion list for zip command | ||
| const excludeArgs = DEFAULT_IGNORE_PATTERNS.flatMap((pattern) => ['-x', pattern]) | ||
| // Use system zip command to create the archive | ||
| try { | ||
| await execa('zip', ['-r', '-q', zipPath, '.', ...excludeArgs], { | ||
| all: true, | ||
| cwd: sourceDir, | ||
| stdio: ['ignore', 'pipe', 'pipe'], | ||
| }) | ||
| } catch (baseErr) { | ||
| // An empty archive is an expected outcome, not a failure — let the caller decide. | ||
| if (isEmptyZipError(baseErr)) { | ||
| throw new EmptySourceZipError() | ||
| } | ||
|
|
||
| // Use system zip command to create the archive | ||
| try { | ||
| await execa('zip', ['-r', '-q', zipPath, '.', ...excludeArgs], { | ||
| all: true, | ||
| cwd: sourceDir, | ||
| stdio: ['ignore', 'pipe', 'pipe'], | ||
| }) | ||
| } catch (_baseErr) { | ||
| let message = 'zip command failed' | ||
| if (_baseErr instanceof Error && 'command' in _baseErr) { | ||
| const baseErr = _baseErr as ExecaError | ||
| message = `${baseErr.shortMessage}\n\n${baseErr.all ?? ''}` | ||
| let message = 'zip command failed' | ||
| if (baseErr instanceof Error && 'command' in baseErr) { | ||
| const execaErr = baseErr as ExecaError | ||
| message = `${execaErr.shortMessage}\n\n${execaErr.all ?? ''}` | ||
| } | ||
| throw new Error(message, { cause: baseErr }) | ||
| } | ||
|
|
||
| return zipPath | ||
| } catch (error) { | ||
| // Nothing usable was produced; drop the temp directory we created. | ||
| await removeSourceZipDir(zipPath) | ||
|
|
||
| // An empty source zip is not a reported failure. | ||
| if (error instanceof EmptySourceZipError) { | ||
| throw error | ||
| } | ||
| throw new Error(message, { cause: _baseErr }) | ||
|
|
||
| const errorMsg = error instanceof Error ? error.message : String(error) | ||
| statusCb({ | ||
| type: 'source-zip-upload', | ||
| msg: `Failed to create source zip: ${errorMsg}`, | ||
| phase: 'error', | ||
| }) | ||
| warn(`Failed to create source zip: ${errorMsg}`) | ||
| throw error | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Relevant files ==\n'
fd -a 'upload-source-zip.ts|deploy.ts' src
printf '\n== Outline: src/utils/deploy/upload-source-zip.ts ==\n'
ast-grep outline src/utils/deploy/upload-source-zip.ts --view expanded
printf '\n== Outline: src/utils/deploy/deploy.ts ==\n'
ast-grep outline src/utils/deploy/deploy.ts --view expanded
printf '\n== Search for createSourceZip / uploadSourceZip usage ==\n'
rg -n "createSourceZip|uploadSourceZip|source_zip_upload_url|source_zip_filename|removeSourceZipDir|temporaryDirectory" src
printf '\n== Read upload-source-zip.ts (selected ranges) ==\n'
sed -n '1,240p' src/utils/deploy/upload-source-zip.ts
printf '\n== Read deploy.ts (selected ranges around source zip handling) ==\n'
rg -n -A40 -B20 "source_zip_upload_url|source_zip_filename|createSourceZip|uploadSourceZip" src/utils/deploy/deploy.tsRepository: netlify/cli
Length of output: 10488
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== deploy.ts source-zip section ==\n'
sed -n '1380,1465p' src/commands/deploy/deploy.ts
printf '\n== createSourceZip callsites ==\n'
rg -n -A8 -B8 "createSourceZip\(" src/commands/deploy/deploy.ts src
printf '\n== cleanup/finally around deploy flow ==\n'
rg -n -A20 -B20 "finally|removeSourceZipDir|uploadSourceZip\(" src/commands/deploy/deploy.tsRepository: netlify/cli
Length of output: 8756
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Any cleanup of source zip path elsewhere ==\n'
rg -n "removeSourceZipDir|rm\\(dirname\\(zipPath\\)|sourceZipPath|sourceZipFileName|uploadSourceZipResult" src/commands/deploy src/utils/deploy
printf '\n== buildSourceZipOrExit implementation ==\n'
sed -n '300,330p' src/commands/deploy/deploy.tsRepository: netlify/cli
Length of output: 3144
Clean up sourceZipPath when the upload branch is skipped src/commands/deploy/deploy.ts:1424
If createSourceZip succeeds but the deploy response never provides both source_zip_upload_url and source_zip_filename (or fails before that branch runs), the temp directory stays on disk because only uploadSourceZip removes it. Add a caller-side cleanup path for the no-upload case.
🤖 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 `@src/utils/deploy/upload-source-zip.ts` around lines 59 - 132, The deploy flow
currently leaves the temporary source zip directory behind when
`createSourceZip` succeeds but the `uploadSourceZip` branch is skipped because
the response lacks both `source_zip_upload_url` and `source_zip_filename`, or an
error occurs before upload. Add a caller-side cleanup path in the deploy logic
that owns `sourceZipPath` so the temp directory is removed whenever no upload
happens, while keeping `uploadSourceZip` responsible for the successful upload
case.
| // Check for Windows - this feature is not supported on Windows | ||
| if (platform() === 'win32') { | ||
| throw new Error('Source zip upload is not supported on Windows') | ||
| } | ||
|
|
||
| // Ensure the directory for the zip file exists | ||
| // The filename from the API includes a subdirectory path (e.g., 'workspace-snapshots/source-xxx.zip') | ||
| // While temporaryDirectory() creates a new empty directory, the subdirectory within it doesn't exist | ||
| // so we need to create it before the zip command can write the file | ||
| await mkdir(dirname(zipPath), { recursive: true }) | ||
| statusCb({ | ||
| type: 'source-zip-upload', | ||
| msg: `Creating source zip...`, | ||
| phase: 'start', | ||
| }) | ||
|
|
||
| statusCb({ | ||
| type: 'source-zip-upload', | ||
| msg: `Creating source zip...`, | ||
| phase: 'start', | ||
| }) | ||
| // Create exclusion list for zip command | ||
| const excludeArgs = DEFAULT_IGNORE_PATTERNS.flatMap((pattern) => ['-x', pattern]) | ||
|
|
||
| // Create exclusion list for zip command | ||
| const excludeArgs = DEFAULT_IGNORE_PATTERNS.flatMap((pattern) => ['-x', pattern]) | ||
| // Use system zip command to create the archive | ||
| try { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove comments that just restate the following line.
"Check for Windows - this feature is not supported on Windows", "Create exclusion list for zip command", and "Use system zip command to create the archive" merely narrate the code immediately below them and add no information the code doesn't already convey.
♻️ Proposed cleanup
- // Check for Windows - this feature is not supported on Windows
if (platform() === 'win32') {
throw new Error('Source zip upload is not supported on Windows')
}
statusCb({
type: 'source-zip-upload',
msg: `Creating source zip...`,
phase: 'start',
})
- // Create exclusion list for zip command
const excludeArgs = DEFAULT_IGNORE_PATTERNS.flatMap((pattern) => ['-x', pattern])
- // Use system zip command to create the archive
try {
await execa('zip', ['-r', '-q', zipPath, '.', ...excludeArgs], {As per coding guidelines, "Never write comments on what the code does; make the code clean and self-explanatory instead."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Check for Windows - this feature is not supported on Windows | |
| if (platform() === 'win32') { | |
| throw new Error('Source zip upload is not supported on Windows') | |
| } | |
| // Ensure the directory for the zip file exists | |
| // The filename from the API includes a subdirectory path (e.g., 'workspace-snapshots/source-xxx.zip') | |
| // While temporaryDirectory() creates a new empty directory, the subdirectory within it doesn't exist | |
| // so we need to create it before the zip command can write the file | |
| await mkdir(dirname(zipPath), { recursive: true }) | |
| statusCb({ | |
| type: 'source-zip-upload', | |
| msg: `Creating source zip...`, | |
| phase: 'start', | |
| }) | |
| statusCb({ | |
| type: 'source-zip-upload', | |
| msg: `Creating source zip...`, | |
| phase: 'start', | |
| }) | |
| // Create exclusion list for zip command | |
| const excludeArgs = DEFAULT_IGNORE_PATTERNS.flatMap((pattern) => ['-x', pattern]) | |
| // Create exclusion list for zip command | |
| const excludeArgs = DEFAULT_IGNORE_PATTERNS.flatMap((pattern) => ['-x', pattern]) | |
| // Use system zip command to create the archive | |
| try { | |
| if (platform() === 'win32') { | |
| throw new Error('Source zip upload is not supported on Windows') | |
| } | |
| statusCb({ | |
| type: 'source-zip-upload', | |
| msg: `Creating source zip...`, | |
| phase: 'start', | |
| }) | |
| const excludeArgs = DEFAULT_IGNORE_PATTERNS.flatMap((pattern) => ['-x', pattern]) | |
| try { | |
| await execa('zip', ['-r', '-q', zipPath, '.', ...excludeArgs], { |
🤖 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 `@src/utils/deploy/upload-source-zip.ts` around lines 78 - 93, Remove the
redundant narrative comments in uploadSourceZip that merely restate the next
statements; keep the logic in platform() check, DEFAULT_IGNORE_PATTERNS
handling, and the zip command try block unchanged, and rely on the code itself
for clarity.
Source: Coding guidelines
1 participant
Problem
When deploying with
--upload-source-zip, the CLI creates the deploy record (createSiteDeploy) before building the source zip. If the zip is empty — every file matched an ignore pattern, or the working tree has no source —zipexits 12 (Nothing to do!). Today that means:--buildpath doesn't evencancelDeploy), andChange
createSiteDeploywhen it's non-empty. An empty source therefore never creates a deploy.zip's own "nothing to do" code) so upstream tooling can treat it as a non-failure rather than a deploy error.uploadSourceZipintocreateSourceZip(throws a typedEmptySourceZipError) +uploadSourceZipFile, pluscleanupSourceZip.Both deploy paths (
--buildand--no-build) are updated.Testing
tsc+eslintclean.EmptySourceZipError; a normal source produces a zip (excluding build output); the temp zip is cleaned up.🤖 Generated with Claude Code