-
-
Notifications
You must be signed in to change notification settings - Fork 1k
feat(webapp/deployments): Vercel improvements & fixes #3037
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
f843359
47f5700
bae6f60
f74bcd4
bbeded8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -47,6 +47,7 @@ import { type VercelOnboardingData } from "~/presenters/v3/VercelSettingsPresent | |
| import { vercelAppInstallPath, v3ProjectSettingsPath, githubAppInstallPath, vercelResourcePath } from "~/utils/pathBuilder"; | ||
| import type { loader } from "~/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.vercel"; | ||
| import { useEffect, useState, useCallback, useRef } from "react"; | ||
| import { usePostHogTracking } from "~/hooks/usePostHog"; | ||
|
|
||
| function safeRedirectUrl(url: string): string | null { | ||
| try { | ||
|
|
@@ -114,6 +115,7 @@ export function VercelOnboardingModal({ | |
| nextUrl?: string; | ||
| onDataReload?: (vercelStagingEnvironment?: string) => void; | ||
| }) { | ||
| const { capture, startSessionRecording } = usePostHogTracking(); | ||
| const navigation = useNavigation(); | ||
| const fetcher = useTypedFetcher<typeof loader>(); | ||
| const envMappingFetcher = useFetcher(); | ||
|
|
@@ -172,6 +174,31 @@ export function VercelOnboardingModal({ | |
| prevIsOpenRef.current = isOpen; | ||
| }, [isOpen, state, computeInitialState]); | ||
|
|
||
| const trackOnboarding = useCallback( | ||
| (eventName: string, extraProperties?: Record<string, unknown>) => { | ||
| capture(eventName, { | ||
| origin: fromMarketplaceContext ? "marketplace" : "dashboard", | ||
| step: state, | ||
| organization_slug: organizationSlug, | ||
| project_slug: projectSlug, | ||
| ...extraProperties, | ||
| }); | ||
| }, | ||
| [capture, fromMarketplaceContext, state, organizationSlug, projectSlug] | ||
| ); | ||
|
|
||
| const hasTrackedStartRef = useRef(false); | ||
| useEffect(() => { | ||
| if (isOpen && state === "project-selection" && !hasTrackedStartRef.current) { | ||
| hasTrackedStartRef.current = true; | ||
| startSessionRecording(); | ||
| trackOnboarding("vercel onboarding started"); | ||
| } | ||
| if (!isOpen) { | ||
| hasTrackedStartRef.current = false; | ||
| } | ||
| }, [isOpen, state, trackOnboarding, startSessionRecording]); | ||
|
|
||
| const [selectedVercelProject, setSelectedVercelProject] = useState<{ | ||
| id: string; | ||
| name: string; | ||
|
|
@@ -337,14 +364,17 @@ export function VercelOnboardingModal({ | |
|
|
||
| useEffect(() => { | ||
| if (state === "project-selection" && fetcher.data && "success" in fetcher.data && fetcher.data.success && fetcher.state === "idle") { | ||
| trackOnboarding("vercel onboarding project selected", { | ||
| vercel_project_name: selectedVercelProject?.name, | ||
| }); | ||
| setState("loading-env-mapping"); | ||
| if (onDataReload) { | ||
| onDataReload(); | ||
| } | ||
| } else if (fetcher.data && "error" in fetcher.data && typeof fetcher.data.error === "string") { | ||
| setProjectSelectionError(fetcher.data.error); | ||
| } | ||
| }, [state, fetcher.data, fetcher.state, onDataReload]); | ||
| }, [state, fetcher.data, fetcher.state, onDataReload, trackOnboarding, selectedVercelProject?.name]); | ||
|
|
||
| // For marketplace origin, skip env-mapping step | ||
| useEffect(() => { | ||
|
|
@@ -449,14 +479,23 @@ export function VercelOnboardingModal({ | |
| method: "post", | ||
| action: actionUrl, | ||
| }); | ||
|
Comment on lines
479
to
481
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚩 trackOnboarding in handleSkipOnboarding dependency array but never called
However, when the user clicks "Cancel" (which calls This may be an oversight where the developer intended to add (Refers to lines 469-481) Was this helpful? React with 👍 or 👎 to provide feedback. |
||
| }, [actionUrl, fetcher, onClose, nextUrl, fromMarketplaceContext]); | ||
| }, [actionUrl, fetcher, onClose, nextUrl, fromMarketplaceContext, trackOnboarding]); | ||
|
|
||
| const handleSkipEnvMapping = useCallback(() => { | ||
| trackOnboarding("vercel onboarding env mapping completed", { | ||
| skipped: true, | ||
| staging_environment: null, | ||
| }); | ||
| setVercelStagingEnvironment(null); | ||
| setState("loading-env-vars"); | ||
| }, []); | ||
| }, [trackOnboarding]); | ||
|
|
||
| const handleUpdateEnvMapping = useCallback(() => { | ||
| trackOnboarding("vercel onboarding env mapping completed", { | ||
| skipped: false, | ||
| staging_environment: vercelStagingEnvironment?.displayName ?? null, | ||
| }); | ||
|
|
||
| if (!vercelStagingEnvironment) { | ||
| setState("loading-env-vars"); | ||
| return; | ||
|
|
@@ -471,9 +510,11 @@ export function VercelOnboardingModal({ | |
| action: actionUrl, | ||
| }); | ||
|
|
||
| }, [vercelStagingEnvironment, envMappingFetcher, actionUrl]); | ||
| }, [vercelStagingEnvironment, envMappingFetcher, actionUrl, trackOnboarding]); | ||
|
|
||
| const handleBuildSettingsNext = useCallback(() => { | ||
| trackOnboarding("vercel onboarding build settings completed"); | ||
|
|
||
| if (nextUrl && fromMarketplaceContext && isGitHubConnectedForOnboarding) { | ||
| setIsRedirecting(true); | ||
| } | ||
|
|
@@ -501,7 +542,7 @@ export function VercelOnboardingModal({ | |
| if (!isGitHubConnectedForOnboarding) { | ||
| setState("github-connection"); | ||
| } | ||
| }, [vercelStagingEnvironment, pullEnvVarsBeforeBuild, atomicBuilds, discoverEnvVars, syncEnvVarsMapping, nextUrl, fromMarketplaceContext, isGitHubConnectedForOnboarding, completeOnboardingFetcher, actionUrl]); | ||
| }, [vercelStagingEnvironment, pullEnvVarsBeforeBuild, atomicBuilds, discoverEnvVars, syncEnvVarsMapping, nextUrl, fromMarketplaceContext, isGitHubConnectedForOnboarding, completeOnboardingFetcher, actionUrl, trackOnboarding]); | ||
|
|
||
| const handleFinishOnboarding = useCallback((e: React.FormEvent<HTMLFormElement>) => { | ||
| e.preventDefault(); | ||
|
|
@@ -531,9 +572,12 @@ export function VercelOnboardingModal({ | |
|
|
||
| useEffect(() => { | ||
| if (state === "completed") { | ||
| trackOnboarding("vercel onboarding completed", { | ||
| github_connected: isGitHubConnectedForOnboarding, | ||
| }); | ||
| onClose(); | ||
| } | ||
| }, [state, onClose]); | ||
| }, [state, onClose, trackOnboarding, isGitHubConnectedForOnboarding]); | ||
|
Comment on lines
573
to
+580
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Potential double-fire of "completed" event if deps change while If Suggested fix+ const hasTrackedCompletedRef = useRef(false);
useEffect(() => {
if (state === "completed") {
- trackOnboarding("vercel onboarding completed", {
- github_connected: isGitHubConnectedForOnboarding,
- });
+ if (!hasTrackedCompletedRef.current) {
+ hasTrackedCompletedRef.current = true;
+ trackOnboarding("vercel onboarding completed", {
+ github_connected: isGitHubConnectedForOnboarding,
+ });
+ }
onClose();
}
}, [state, onClose, trackOnboarding, isGitHubConnectedForOnboarding]);Reset the ref when the modal reopens (alongside the existing if (!isOpen) {
hasTrackedStartRef.current = false;
+ hasTrackedCompletedRef.current = false;
}🤖 Prompt for AI Agents |
||
|
|
||
| useEffect(() => { | ||
| if (state === "installing") { | ||
|
|
@@ -565,6 +609,12 @@ export function VercelOnboardingModal({ | |
| } | ||
| }, [state, customEnvironments, vercelStagingEnvironment]); | ||
|
|
||
| useEffect(() => { | ||
| if (state === "project-selection" && availableProjects.length > 0 && !selectedVercelProject) { | ||
| setSelectedVercelProject(availableProjects[0]); | ||
| } | ||
| }, [state, availableProjects, selectedVercelProject]); | ||
|
|
||
| if (!isOpen || onboardingData?.authInvalid) { | ||
| return null; | ||
| } | ||
|
|
@@ -578,7 +628,14 @@ export function VercelOnboardingModal({ | |
|
|
||
| if (isLoadingState) { | ||
| return ( | ||
| <Dialog open={isOpen} onOpenChange={(open) => !open && !fromMarketplaceContext && onClose()}> | ||
| <Dialog open={isOpen} onOpenChange={(open) => { | ||
| if (!open && !fromMarketplaceContext) { | ||
| if (state as string !== "completed") { | ||
| trackOnboarding("vercel onboarding abandoned"); | ||
| } | ||
| onClose(); | ||
| } | ||
| }}> | ||
| <DialogContent className="max-w-lg"> | ||
| <DialogHeader> | ||
| <div className="flex items-center gap-2"> | ||
|
|
@@ -601,7 +658,14 @@ export function VercelOnboardingModal({ | |
| const showGitHubConnection = state === "github-connection"; | ||
|
|
||
| return ( | ||
| <Dialog open={isOpen} onOpenChange={(open) => !open && !fromMarketplaceContext && onClose()}> | ||
| <Dialog open={isOpen} onOpenChange={(open) => { | ||
| if (!open && !fromMarketplaceContext) { | ||
| if (state !== "completed") { | ||
| trackOnboarding("vercel onboarding abandoned"); | ||
0ski marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| onClose(); | ||
| } | ||
| }}> | ||
| <DialogContent className="max-w-lg"> | ||
| <DialogHeader> | ||
| <div className="flex items-center gap-2"> | ||
|
|
@@ -625,6 +689,7 @@ export function VercelOnboardingModal({ | |
| </Callout> | ||
| ) : ( | ||
| <Select | ||
| disabled={availableProjects.length === 1} | ||
| value={selectedVercelProject?.id || ""} | ||
| setValue={(value) => { | ||
| if (!Array.isArray(value)) { | ||
|
|
@@ -634,6 +699,7 @@ export function VercelOnboardingModal({ | |
| } | ||
| }} | ||
| items={availableProjects} | ||
| filter={availableProjects.length > 5 ? { keys: ["name"] } : undefined} | ||
| variant="tertiary/medium" | ||
| placeholder="Select a Vercel project" | ||
| dropdownIcon | ||
|
|
@@ -894,6 +960,10 @@ export function VercelOnboardingModal({ | |
| <Button | ||
| variant="primary/medium" | ||
| onClick={() => { | ||
| trackOnboarding("vercel onboarding env vars configured", { | ||
| env_vars_enabled: enabledEnvVars.length, | ||
| env_vars_total: syncableEnvVars.length, | ||
| }); | ||
| if (fromMarketplaceContext) { | ||
| handleBuildSettingsNext(); | ||
| } else { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,12 @@ import { type AuthenticatedEnvironment } from "~/services/apiAuth.server"; | |
| import { BaseService } from "./baseService.server"; | ||
| import { errAsync, fromPromise, okAsync, type ResultAsync } from "neverthrow"; | ||
| import { type WorkerDeployment, type Project } from "@trigger.dev/database"; | ||
| import { BuildServerMetadata, logger, type GitMeta, type DeploymentEvent } from "@trigger.dev/core/v3"; | ||
| import { | ||
| BuildServerMetadata, | ||
| logger, | ||
| type GitMeta, | ||
| type DeploymentEvent, | ||
| } from "@trigger.dev/core/v3"; | ||
| import { TimeoutDeploymentService } from "./timeoutDeployment.server"; | ||
| import { env } from "~/env.server"; | ||
| import { createRemoteImageBuild } from "../remoteImageBuilder.server"; | ||
|
|
@@ -38,9 +43,20 @@ export class DeploymentService extends BaseService { | |
| public progressDeployment( | ||
| authenticatedEnv: AuthenticatedEnvironment, | ||
| friendlyId: string, | ||
| updates: Partial<Pick<WorkerDeployment, "contentHash" | "runtime"> & { git: GitMeta }> | ||
| updates: Partial< | ||
| Pick<WorkerDeployment, "contentHash" | "runtime"> & { | ||
| git: GitMeta; | ||
| buildServerMetadata: BuildServerMetadata; | ||
| } | ||
| > | ||
| ) { | ||
| const validateDeployment = (deployment: Pick<WorkerDeployment, "id" | "status"> & { buildServerMetadata?: BuildServerMetadata }) => { | ||
| const { buildServerMetadata: newBuildServerMetadata, ...restUpdates } = updates; | ||
|
|
||
| const validateDeployment = ( | ||
| deployment: Pick<WorkerDeployment, "id" | "status"> & { | ||
| buildServerMetadata?: BuildServerMetadata; | ||
| } | ||
| ) => { | ||
| if (deployment.status !== "PENDING" && deployment.status !== "INSTALLING") { | ||
| logger.warn( | ||
| "Attempted progressing deployment that is not in PENDING or INSTALLING status", | ||
|
|
@@ -54,12 +70,27 @@ export class DeploymentService extends BaseService { | |
| return okAsync(deployment); | ||
| }; | ||
|
|
||
| const progressToInstalling = (deployment: Pick<WorkerDeployment, "id">) => | ||
| fromPromise( | ||
| const progressToInstalling = ( | ||
| deployment: Pick<WorkerDeployment, "id"> & { buildServerMetadata?: BuildServerMetadata } | ||
| ) => { | ||
| const existingBuildServerMetadata = deployment.buildServerMetadata as | ||
| | BuildServerMetadata | ||
| | null | ||
| | undefined; | ||
|
|
||
| return fromPromise( | ||
| this._prisma.workerDeployment.updateMany({ | ||
| where: { id: deployment.id, status: "PENDING" }, // status could've changed in the meantime, we're not locking the row | ||
| data: { | ||
| ...updates, | ||
| ...restUpdates, | ||
| ...(newBuildServerMetadata | ||
| ? { | ||
| buildServerMetadata: { | ||
| ...(existingBuildServerMetadata ?? {}), | ||
| ...newBuildServerMetadata, | ||
| }, | ||
| } | ||
| : {}), | ||
| status: "INSTALLING", | ||
| startedAt: new Date(), | ||
| }, | ||
|
|
@@ -74,6 +105,7 @@ export class DeploymentService extends BaseService { | |
| } | ||
| return okAsync({ id: deployment.id, status: "INSTALLING" as const }); | ||
| }); | ||
| }; | ||
|
|
||
| const progressToBuilding = ( | ||
| deployment: Pick<WorkerDeployment, "id"> & { buildServerMetadata?: BuildServerMetadata } | ||
|
|
@@ -85,13 +117,26 @@ export class DeploymentService extends BaseService { | |
| cause: error, | ||
| })); | ||
|
|
||
| const existingBuildServerMetadata = deployment.buildServerMetadata as | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: can avoid the typeassertion by doing a safe parse in |
||
| | BuildServerMetadata | ||
| | null | ||
| | undefined; | ||
|
|
||
| return createRemoteBuildIfNeeded | ||
| .andThen((externalBuildData) => | ||
| fromPromise( | ||
| this._prisma.workerDeployment.updateMany({ | ||
| where: { id: deployment.id, status: "INSTALLING" }, // status could've changed in the meantime, we're not locking the row | ||
| data: { | ||
| ...updates, | ||
| ...restUpdates, | ||
| ...(newBuildServerMetadata | ||
| ? { | ||
| buildServerMetadata: { | ||
| ...(existingBuildServerMetadata ?? {}), | ||
| ...newBuildServerMetadata, | ||
| }, | ||
| } | ||
| : {}), | ||
| externalBuildData, | ||
| status: "BUILDING", | ||
| installedAt: new Date(), | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.