Fix/update publish function #2643
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
apps/web/client/src/server/api/routers/domain/adapters/freestyle.ts
Outdated
Show resolved
Hide resolved
spartan-vutrannguyen
force-pushed
the
fix/update-publish-function
branch
from
85a63d2 to
87078ea
Compare
|
|
||
| const artifactLocalPath = `${CUSTOM_OUTPUT_DIR}/standalone.tar.gz`; | ||
| const tarCommand = `tar -czf ${artifactLocalPath} -C ${NEXT_BUILD_OUTPUT_PATH} .`; | ||
| await this.provider.runCommand({ args: { command: tarCommand } }); |
There was a problem hiding this comment.
Consider using a safer method (e.g. spawning a process with argument arrays) to execute the tar command to mitigate potential shell injection risks.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughIntroduces URL-based deployments via artifacts, with fallback to file-based deployments. Refactors deployment request types to a discriminated union ("url" | "files"). Updates publish flow to build and upload a tar artifact first, then deploy by URL; if that fails, builds files and deploys them. Adapts adapters and helpers accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PublishHelper
participant PublishManager
participant Provider
participant FreestyleAdapter
participant HostingSDK
Client->>PublishHelper: publish(urls, envVars)
PublishHelper->>PublishManager: buildAndUploadArtifact(...)
PublishManager->>Provider: prepareProject + build + tar + downloadFiles
Provider-->>PublishManager: artifact URL
PublishManager-->>PublishHelper: sourceUrl
PublishHelper->>FreestyleAdapter: deploy({type:'url', sourceUrl, config})
FreestyleAdapter->>HostingSDK: deployWeb(kind:'tar', url, config)
alt Artifact path fails
PublishHelper->>PublishManager: buildFiles(...)
PublishManager-->>PublishHelper: files map
PublishHelper->>FreestyleAdapter: deploy({type:'files', files, config})
FreestyleAdapter->>HostingSDK: deployWeb(kind:'files', files, config)
end
HostingSDK-->>FreestyleAdapter: response
FreestyleAdapter-->>PublishHelper: DeploymentResponse
PublishHelper-->>Client: result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Assessment against linked issues
Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/web/client/src/server/api/routers/domain/adapters/freestyle.ts (1)
25-29: Previous review comment about operator precedence still applies.Consider adding parentheses around the OR expression for clarity:
- freestyleResponse.error.message ?? freestyleResponse.message ?? 'Unknown error', + (freestyleResponse.error.message || freestyleResponse.message) ?? 'Unknown error',
🧹 Nitpick comments (5)
apps/web/client/src/server/api/routers/publish/helpers/deploy.ts (2)
17-26: Complex type definition could be simplified for maintainability.The inline discriminated union type definition is complex. Consider extracting it to a named type for better readability and reusability.
Add a type definition above the function:
type DeploymentArgs = { urls: string[]; envVars?: Record<string, string>; } & ( | { type: 'url'; sourceUrl: string; files?: never } | { type: 'files'; files: Record<string, FreestyleFile>; sourceUrl?: never } );Then simplify the function signature:
-export const deployFreestyle = async ( - args: ( - { files: Record<string, FreestyleFile>; sourceUrl?: undefined } | - { files?: undefined; sourceUrl: string } - ) & { - urls: string[]; - envVars?: Record<string, string>; - type: "url" | "files"; - } -): Promise<{ +export const deployFreestyle = async ( + args: DeploymentArgs +): Promise<{
35-62: Type guards could be more robust.The current type checking relies on both the
typefield and the presence of data fields. This dual checking is redundant given the discriminated union design.Simplify the conditions to rely solely on the discriminator:
- if(args.type === 'url' && args.sourceUrl) { + if(args.type === 'url') { result = await adapter.deploy({ type: 'url', sourceUrl: args.sourceUrl,- } else if(args.type === 'files' && args.files) { + } else if(args.type === 'files') { result = await adapter.deploy({ type: 'files', files: Object.fromEntries(apps/web/client/src/server/api/routers/publish/helpers/publish.ts (3)
82-83: Consider extracting duplicated environment variable merging logic.The environment variable extraction and merging logic is duplicated between the artifact and file deployment paths.
Extract to a helper function at the top of the file:
async function getMergedEnvVars( provider: Provider, userEnvVars?: Record<string, string> ): Promise<Record<string, string>> { const sandboxEnvVars = await extractEnvVarsFromSandbox(provider); return { ...sandboxEnvVars, ...(userEnvVars ?? {}) }; }Then use it in both places:
- const sandboxEnvVars = await extractEnvVarsFromSandbox(provider); - const mergedEnvVars = { ...sandboxEnvVars, ...(envVars ?? {}) }; + const mergedEnvVars = await getMergedEnvVars(provider, envVars);Also applies to: 116-117
92-94: Consider more structured error handling for the fallback mechanism.While the warning log is helpful, consider adding metrics or alerts for monitoring the fallback rate in production, as frequent fallbacks might indicate infrastructure issues.
Consider adding:
- Structured logging with error categorization
- Metrics tracking for fallback occurrences
- Alerting thresholds if fallback rate exceeds acceptable limits
69-79: Duplicated deployment update logic could be extracted.The deployment status update to "Deploying build..." at 80% progress is duplicated between both paths.
Extract to a helper function:
async function updateToDeployingStatus( db: DrizzleDb, deploymentId: string ): Promise<void> { const result = await updateDeployment(db, deploymentId, { status: DeploymentStatus.IN_PROGRESS, message: 'Deploying build...', progress: 80, }); if (!result) { throw new TRPCError({ code: 'BAD_REQUEST', message: 'Update deployment failed', }); } }Then use in both places:
- const updateDeploymentResult3 = await updateDeployment(db, deploymentId, { - status: DeploymentStatus.IN_PROGRESS, - message: 'Deploying build...', - progress: 80, - }); - if (!updateDeploymentResult3) { - throw new TRPCError({ - code: 'BAD_REQUEST', - message: 'Update deployment failed', - }); - } + await updateToDeployingStatus(db, deploymentId);Also applies to: 104-114
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/web/client/src/server/api/routers/domain/adapters/freestyle.ts(1 hunks)apps/web/client/src/server/api/routers/publish/helpers/deploy.ts(2 hunks)apps/web/client/src/server/api/routers/publish/helpers/publish.ts(1 hunks)apps/web/client/src/server/api/routers/publish/helpers/unpublish.ts(1 hunks)apps/web/client/src/server/api/routers/publish/manager.ts(4 hunks)packages/models/src/hosting/index.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
apps/web/client/src/server/api/routers/domain/adapters/freestyle.ts (2)
packages/models/src/hosting/index.ts (3)
HostingProviderAdapter(87-89)DeploymentRequest(79-79)DeploymentResponse(81-85)apps/web/client/src/server/api/routers/domain/freestyle.ts (1)
initializeFreestyleSdk(5-15)
apps/web/client/src/server/api/routers/publish/helpers/publish.ts (4)
packages/constants/src/editor.ts (1)
DefaultSettings(38-70)apps/web/client/src/server/api/routers/publish/helpers/helpers.ts (1)
updateDeployment(55-72)apps/web/client/src/server/api/routers/publish/helpers/env.ts (1)
extractEnvVarsFromSandbox(41-68)apps/web/client/src/server/api/routers/publish/helpers/deploy.ts (1)
deployFreestyle(17-69)
apps/web/client/src/server/api/routers/publish/helpers/deploy.ts (2)
apps/web/client/src/server/api/routers/domain/hosting-factory.ts (1)
HostingProviderFactory(4-13)packages/models/src/hosting/index.ts (1)
DeploymentResponse(81-85)
apps/web/client/src/server/api/routers/publish/manager.ts (7)
packages/utility/src/file.ts (2)
FileOperations(17-28)updateGitignore(36-72)packages/growth/src/inject.ts (2)
injectBuiltWithScript(11-148)addBuiltWithScript(155-177)packages/parser/src/code-edit/next-config.ts (1)
addNextBuildConfig(182-247)packages/constants/src/files.ts (1)
SUPPORTED_LOCK_FILES(23-28)apps/web/client/src/server/api/routers/publish/helpers/helpers.ts (1)
updateDeployment(55-72)apps/web/client/src/utils/git/index.ts (1)
escapeShellString(53-66)packages/db/src/schema/project/deployment.ts (2)
deploymentUpdateSchema(48-48)Deployment(50-50)
🔇 Additional comments (24)
packages/models/src/hosting/index.ts (2)
64-67: LGTM! Well-structured discriminated union pattern.The base interface approach with discriminated union types is clean and follows TypeScript best practices. This pattern enables type-safe handling throughout the deployment pipeline.
69-77: LGTM! Clear type discrimination for deployment modes.The separation between
FileDeploymentRequestandUrlDeploymentRequestwith explicit type literals enables proper exhaustiveness checking at compile time.apps/web/client/src/server/api/routers/domain/adapters/freestyle.ts (3)
3-7: LGTM! Import style adjustment aligns with TypeScript conventions.The change to import types alongside regular imports is clean and consistent.
13-36: LGTM! URL-based deployment path properly implemented.The new URL deployment branch correctly handles the discriminated union type and provides consistent error handling with appropriate fallback messages using nullish coalescing.
38-44: Default files object prevents potential runtime errors.Good defensive programming - defaulting
request.filesto{}ensures the SDK receives a valid object even when files are undefined.apps/web/client/src/server/api/routers/publish/helpers/unpublish.ts (1)
24-29: LGTM! Correct usage of the new deployment API.The unpublish function correctly uses the
type: 'files'mode with an empty files object, which is appropriate for removing deployments.apps/web/client/src/server/api/routers/publish/helpers/publish.ts (1)
60-94: Good fallback strategy for memory optimization.The try-catch pattern with artifact-first deployment and file-based fallback directly addresses the memory consumption issue from #2505. The artifact approach using provider-signed URLs avoids loading entire repositories into memory.
apps/web/client/src/server/api/routers/publish/manager.ts (17)
12-12: LGTM - Import added for shell command safety.The import of
escapeShellStringis appropriate for safely constructing shell commands later in the file.
22-23: LGTM - Type-only imports for better tree-shaking.Using type-only imports for
FreestyleFileandProvideris a good practice that can improve bundle size.
26-26: LGTM - Clean constructor injection.Constructor injection of the provider follows good dependency injection patterns.
28-65: LGTM - Consistent provider API usage.The refactoring to use a single
argsobject for all provider calls improves consistency. The file operations abstraction is well-maintained.
67-70: LGTM - Good separation of concerns.The
addBadgemethod provides a clear abstraction for badge-related operations.
72-81: LGTM - Proper error handling in project preparation.The method throws on critical failures while logging warnings for non-critical ones (gitignore update). This is appropriate behavior.
83-99: LGTM - Robust build step implementation.Good handling of build flags with sensible defaults, and proper error propagation.
101-145: LGTM - Comprehensive postprocessing with clear error reporting.The method properly validates the build output, copies necessary files, and handles lock file requirements with clear error messages.
147-159: LGTM - Well-structured artifact upload method.The method signature is clear and follows the established pattern from the codebase.
224-229: LGTM - Proper error handling for download URL.Good validation that the download URL is available before proceeding.
237-240: Good documentation of artifact lifecycle.The comment clearly explains why the artifact isn't deleted immediately, preventing potential race conditions.
242-301: LGTM - Consistent pattern for file-based deployment.The
buildFilesmethod follows the same structure asbuildAndUploadArtifactbut returns serialized files instead of a URL. Good separation of concerns.
308-340: LGTM - Well-architected file serialization.The method properly handles batching, categorization, and error handling. The parallel processing approach should help with performance.
342-373: LGTM - Efficient directory traversal.Using a queue-based approach for directory traversal is more memory-efficient than recursion, and properly excludes heavy directories early.
377-387: LGTM - Comprehensive file filtering.The skip conditions cover all the typical directories and files that should be excluded from deployment.
389-488: LGTM - Robust batch processing implementation.Both text and binary file processing methods handle errors gracefully and use appropriate encodings. The batch size of 50 is reasonable for balancing memory usage and performance.
203-215: Shell argument escaping is robust and prevents injection.The
escapeShellStringfunction correctly:
- Returns
""for empty or non-string inputs.- Leaves only safe characters (A–Z, a–z, 0–9,
.,_,-,/) unquoted.- Wraps all other inputs in single-quotes, escaping any embedded
'as'\''.This ensures that each
tarargument cannot break out of its intended value. No further changes are needed here.
|
Publishing fails for me in a brand new project |
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Before:
After:
Additional Notes
Important
Update publish function to use provider-signed download URLs for deployments, supporting both file and URL-based methods.
publish.tsanddeploy.ts.deployFreestyle().DeploymentRequesttype to handle 'url' and 'files'.FreestyleAdapterinfreestyle.tsto handle URL deployments.PublishManagerinmanager.tsto build and upload artifacts using URLs.publish.ts.deployFreestyle()to merge environment variables from sandbox and user input.This description was created by
for e8df827. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
Bug Fixes