diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index adabec784..6257bbdde 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -7,6 +7,8 @@ # Core packages/core/src/helpers/oauth-request.ts @DataDog/app-builder-high-code @DataDog/build-plugins packages/core/src/helpers/oauth-request.test.ts @DataDog/app-builder-high-code @DataDog/build-plugins +packages/core/src/helpers/site.ts @DataDog/app-builder-high-code @DataDog/build-plugins +packages/core/src/helpers/site.test.ts @DataDog/app-builder-high-code @DataDog/build-plugins # Build Report packages/plugins/build-report @DataDog/build-plugins diff --git a/MIGRATIONS.md b/MIGRATIONS.md index 479d05af7..dd82adf17 100644 --- a/MIGRATIONS.md +++ b/MIGRATIONS.md @@ -243,7 +243,7 @@ This replaces the individual endpoint configurations at the product level. } ``` -Supported `site` include: `'datadoghq.com'` (default), `'datadoghq.eu'`, `'us3.datadoghq.com'`, `'us5.datadoghq.com'`, `'ap1.datadoghq.com'`, etc. +Supported `site` include: `'datadoghq.com'` (default), `'datadoghq.eu'`, `'us3.datadoghq.com'`, `'us5.datadoghq.com'`, `'ap1.datadoghq.com'`, etc. You can also prefix any of these with a custom subdomain (e.g. `'myorg.us5.datadoghq.com'`) if your organization uses a custom Datadog URL. > [!NOTE] > - You can still use `DATADOG_SOURCEMAP_INTAKE_URL` to override the sourcemaps' intake url. diff --git a/README.md b/README.md index 0daa72217..d51a23df1 100644 --- a/README.md +++ b/README.md @@ -179,6 +179,8 @@ The Datadog site to use APIs from, and which Datadog site telemetry metrics and - `'ap1.datadoghq.com'` - `'ap2.datadoghq.com'` +You can also prefix any of the sites above with a custom subdomain (e.g. `'myorg.us5.datadoghq.com'`), if your organization uses a custom Datadog URL. All API calls (metrics, error tracking sourcemaps, Apps uploads) still go to the base site; the subdomain is only used to send you to your org's custom URL during the [Apps OAuth flow](/packages/plugins/apps#appsauthoverridesmethod). + An unsupported value (passed via configuration or the `DATADOG_SITE` / `DD_SITE` environment variable) will throw at plugin initialization. > [!NOTE] diff --git a/packages/core/src/helpers/oauth-request.test.ts b/packages/core/src/helpers/oauth-request.test.ts index 210573b8c..c39304d6b 100644 --- a/packages/core/src/helpers/oauth-request.test.ts +++ b/packages/core/src/helpers/oauth-request.test.ts @@ -193,6 +193,15 @@ describe('Core - OAuth', () => { expect(config.authorizationUrl).toBe('https://app.datad0g.com/oauth2/v1/authorize'); expect(config.tokenUrl).toBe('https://api.datad0g.com/oauth2/v1/token'); }); + + test('Should direct the authorization URL to a custom subdomain when given one', () => { + const config = getDatadogOAuthConfig('us5.datadoghq.com', 'customsubdomain'); + expect(config.authorizationUrl).toBe( + 'https://customsubdomain.us5.datadoghq.com/oauth2/v1/authorize', + ); + // The token exchange always stays on the base site's API host. + expect(config.tokenUrl).toBe('https://api.us5.datadoghq.com/oauth2/v1/token'); + }); }); describe('resolveOAuthToken', () => { diff --git a/packages/core/src/helpers/oauth-request.ts b/packages/core/src/helpers/oauth-request.ts index c2680cd4f..04f7c6dac 100644 --- a/packages/core/src/helpers/oauth-request.ts +++ b/packages/core/src/helpers/oauth-request.ts @@ -93,9 +93,12 @@ export const getOAuthClientId = (site: string) => { } }; -export const getDatadogOAuthConfig = (site: string): OAuthConfig => { +export const getDatadogOAuthConfig = (site: string, subdomain?: string): OAuthConfig => { const clientId = getOAuthClientId(site); - const authorizationUrl = `https://app.${site}/oauth2/v1/authorize`; + // Custom subdomains are a web-UI alias for the org's "app." host; API/token + // calls always stay on the base site. + const authorizationHost = subdomain ? `${subdomain}.${site}` : `app.${site}`; + const authorizationUrl = `https://${authorizationHost}/oauth2/v1/authorize`; const tokenUrl = `https://api.${site}/oauth2/v1/token`; return { @@ -632,8 +635,8 @@ export const getOAuthToken = async ( return token; }; -// Memoize per site+client for the lifetime of the process so concurrent requests -// (and the sequential upload + release calls) share a single browser authorization. +// Memoize per site+subdomain+client for the lifetime of the process so concurrent +// requests (and the sequential upload + release calls) share a single browser authorization. const tokenCache = new Map>(); const resolveOAuthTokenFromStorage = async ( @@ -650,9 +653,13 @@ const resolveOAuthTokenFromStorage = async ( return { persistAfterSuccessfulRequest: true, token }; }; -export const resolveOAuthToken = async (site: string, log: Logger): Promise => { - const options = getDatadogOAuthConfig(site); - const key = `${site}:${options.clientId}`; +export const resolveOAuthToken = async ( + site: string, + log: Logger, + subdomain?: string, +): Promise => { + const options = getDatadogOAuthConfig(site, subdomain); + const key = `${site}:${subdomain ?? ''}:${options.clientId}`; let pending = tokenCache.get(key); if (!pending) { pending = resolveOAuthTokenFromStorage(site, options, log).catch((error) => { @@ -670,8 +677,12 @@ export type OAuthRequestOpts = Omit & { }; export const doOAuthRequest = async ({ auth, log, ...opts }: OAuthRequestOpts): Promise => { - const { site } = auth; - const { persistAfterSuccessfulRequest, token } = await resolveOAuthToken(site, log); + const { site, siteSubdomain } = auth; + const { persistAfterSuccessfulRequest, token } = await resolveOAuthToken( + site, + log, + siteSubdomain, + ); if (!token.accessToken) { throw new Error('OAuth authentication did not return an access token.'); @@ -685,7 +696,7 @@ export const doOAuthRequest = async ({ auth, log, ...opts }: OAuthRequestOpts }); if (persistAfterSuccessfulRequest) { - await saveOAuthToken(token, getDatadogOAuthConfig(site), log, site); + await saveOAuthToken(token, getDatadogOAuthConfig(site, siteSubdomain), log, site); } return result; diff --git a/packages/core/src/helpers/site.test.ts b/packages/core/src/helpers/site.test.ts new file mode 100644 index 000000000..aa54c5dc8 --- /dev/null +++ b/packages/core/src/helpers/site.test.ts @@ -0,0 +1,66 @@ +// Unless explicitly stated otherwise all files in this repository are licensed under the MIT License. +// This product includes software developed at Datadog (https://www.datadoghq.com/). +// Copyright 2019-Present Datadog, Inc. + +import { parseSite } from '@dd/core/helpers/site'; + +describe('Core Helpers', () => { + describe('parseSite', () => { + const cases = [ + { + description: 'accept a bare known site unchanged', + input: 'us5.datadoghq.com', + expected: { site: 'us5.datadoghq.com' }, + }, + { + description: 'accept a custom subdomain on top of a known site', + input: 'customsubdomain.us5.datadoghq.com', + expected: { site: 'us5.datadoghq.com', subdomain: 'customsubdomain' }, + }, + { + description: 'accept a custom subdomain on top of a bare site', + input: 'foobar.datadoghq.com', + expected: { site: 'datadoghq.com', subdomain: 'foobar' }, + }, + { + description: 'prefer the longest (most specific) matching base site', + input: 'myorg.us5.datadoghq.com', + expected: { site: 'us5.datadoghq.com', subdomain: 'myorg' }, + }, + { + description: 'match case-insensitively and normalize to lowercase', + input: 'CustomSubdomain.US5.DatadogHQ.com', + expected: { site: 'us5.datadoghq.com', subdomain: 'customsubdomain' }, + }, + { + description: 'reject a site that is not a known site or subdomain of one', + input: 'not-a-real-site.example.com', + expected: undefined, + }, + { + description: 'reject a subdomain with multiple labels', + input: 'foo.bar.datadoghq.com', + expected: undefined, + }, + { + description: 'reject a subdomain with invalid characters', + input: 'foo_bar.datadoghq.com', + expected: undefined, + }, + { + description: 'reject a non-string value instead of throwing', + input: 123 as unknown as string, + expected: undefined, + }, + { + description: 'reject a null value instead of throwing', + input: null as unknown as string, + expected: undefined, + }, + ]; + + test.each(cases)('should $description', ({ input, expected }) => { + expect(parseSite(input)).toEqual(expected); + }); + }); +}); diff --git a/packages/core/src/helpers/site.ts b/packages/core/src/helpers/site.ts new file mode 100644 index 000000000..227c65761 --- /dev/null +++ b/packages/core/src/helpers/site.ts @@ -0,0 +1,40 @@ +// Unless explicitly stated otherwise all files in this repository are licensed under the MIT License. +// This product includes software developed at Datadog (https://www.datadoghq.com/). +// Copyright 2019-Present Datadog, Inc. + +import { SITES } from '@dd/core/constants'; + +// A site value can be a bare known site (e.g. "datadoghq.com") or a single-label +// custom subdomain on top of one (e.g. "myorg.us5.datadoghq.com"), used by orgs +// with a custom Datadog URL. Picks the longest (most specific) matching base site +// so "myorg.us5.datadoghq.com" resolves to "us5.datadoghq.com", not "datadoghq.com". +// Matching is case-insensitive; the returned site and subdomain are normalized to lowercase. +export const parseSite = ( + value: string, +): { site: (typeof SITES)[number]; subdomain?: string } | undefined => { + // Config isn't enforced by the type system at runtime (e.g. plain JS configs), + // so a non-string can reach here despite the `string` signature. + if (typeof value !== 'string') { + return undefined; + } + + const lowerValue = value.toLowerCase(); + + const exactMatch = SITES.find((s) => s === lowerValue); + if (exactMatch) { + return { site: exactMatch }; + } + + const suffixMatches = SITES.filter((s) => lowerValue.endsWith(`.${s}`)); + if (!suffixMatches.length) { + return undefined; + } + + const site = suffixMatches.reduce((longest, s) => (s.length > longest.length ? s : longest)); + const subdomain = lowerValue.slice(0, lowerValue.length - site.length - 1); + if (!subdomain || !/^[a-z0-9-]+$/.test(subdomain)) { + return undefined; + } + + return { site, subdomain }; +}; diff --git a/packages/core/src/types.ts b/packages/core/src/types.ts index 63e8fab9f..2f2a801fe 100644 --- a/packages/core/src/types.ts +++ b/packages/core/src/types.ts @@ -261,7 +261,13 @@ export type AuthOptions = { site?: string; }; -export type AuthOptionsWithDefaults = Omit & { site: Site }; +export type AuthOptionsWithDefaults = Omit & { + site: Site; + // A single-label custom subdomain on top of `site`, e.g. "myorg" for + // "myorg.us5.datadoghq.com". Only used to direct the user to their org's + // custom URL in the OAuth authorization step; API calls stay on `site`. + siteSubdomain?: string; +}; export interface BaseOptions { auth?: AuthOptions; diff --git a/packages/factory/src/validate.test.ts b/packages/factory/src/validate.test.ts index 18da6de83..81306a366 100644 --- a/packages/factory/src/validate.test.ts +++ b/packages/factory/src/validate.test.ts @@ -41,6 +41,38 @@ describe('factory validateOptions', () => { }); }); + // Exercises validateOptions' own wiring (defaulting, error aggregation) around + // parseSite. Site-format parsing itself (subdomains, case-insensitivity, rejection + // rules) is unit-tested directly in `@dd/core/helpers/site`'s own test file. + describe('auth.site', () => { + it('should default to the default site when unset', () => { + const result = validateOptions(); + expect(result.auth.site).toBe('datadoghq.com'); + expect(result.auth.siteSubdomain).toBeUndefined(); + }); + + it('should accept a custom subdomain on top of a known site', () => { + const result = validateOptions({ + auth: { site: 'customsubdomain.us5.datadoghq.com' }, + }); + expect(result.auth.site).toBe('us5.datadoghq.com'); + expect(result.auth.siteSubdomain).toBe('customsubdomain'); + }); + + it('should reject a site that is not a known site or subdomain of one with an aggregated validation error', () => { + expect(() => + validateOptions({ auth: { site: 'not-a-real-site.example.com' } }), + ).toThrow(/auth\.site.*is not a supported Datadog site/); + }); + + it('should reject a non-string site value with the standard validation error, not throw internally', () => { + expect(() => + // Plain JS configs aren't enforced by the type system at runtime. + validateOptions({ auth: { site: 123 as unknown as string } }), + ).toThrow(/auth\.site.*is not a supported Datadog site/); + }); + }); + describe('metadata validation', () => { const cases = [ { diff --git a/packages/factory/src/validate.ts b/packages/factory/src/validate.ts index 50f4458b6..a2084dc38 100644 --- a/packages/factory/src/validate.ts +++ b/packages/factory/src/validate.ts @@ -2,30 +2,29 @@ // This product includes software developed at Datadog (https://www.datadoghq.com/). // Copyright 2019-Present Datadog, Inc. -import { DEFAULT_SITE, SITES } from '@dd/core/constants'; +import { DEFAULT_SITE } from '@dd/core/constants'; import { getDDEnvValue } from '@dd/core/helpers/env'; +import { parseSite } from '@dd/core/helpers/site'; import type { AuthOptionsWithDefaults, BuildMetadata, Options, OptionsWithDefaults, - Site, } from '@dd/core/types'; const SITES_DOC_URL = 'https://docs.datadoghq.com/getting_started/site/'; -const isSite = (value: string): value is Site => SITES.some((s) => s === value); - const resolveSite = ( value: string | undefined, source: string, errors: string[], -): Site | undefined => { +): ReturnType => { if (value === undefined) { return undefined; } - if (isSite(value)) { - return value; + const parsed = parseSite(value); + if (parsed) { + return parsed; } errors.push( `${source} "${value}" is not a supported Datadog site. See the site parameters in ${SITES_DOC_URL}.`, @@ -54,10 +53,13 @@ export const validateOptions = (options: Options = {}): OptionsWithDefaults => { // auth.site when no env var is set, so a stale auth.site can't block a // build that has already opted into an env override. const envRaw = getDDEnvValue('SITE'); - const envSite = resolveSite(envRaw, 'DATADOG_SITE/DD_SITE', errors); + const resolvedSite = + resolveSite(envRaw, 'DATADOG_SITE/DD_SITE', errors) ?? + resolveSite(options.auth?.site, 'auth.site', errors); const auth: AuthOptionsWithDefaults = { - site: envSite ?? resolveSite(options.auth?.site, 'auth.site', errors) ?? DEFAULT_SITE, + site: resolvedSite?.site ?? DEFAULT_SITE, + siteSubdomain: resolvedSite?.subdomain, }; if (errors.length) { diff --git a/packages/plugins/apps/README.md b/packages/plugins/apps/README.md index 7b232261e..c9a6480bf 100644 --- a/packages/plugins/apps/README.md +++ b/packages/plugins/apps/README.md @@ -98,7 +98,7 @@ When the method is `oauth`, the plugin derives OAuth client settings from the re For first-time authorization, the plugin starts a temporary local HTTP callback server, opens Datadog authorization in the browser, exchanges the authorization code with PKCE, and saves the returned token response for later uploads. -OAuth token and authorization URLs are derived from `auth.site`, so it must match your Datadog data center (e.g. `datadoghq.com`, `us5.datadoghq.com`, `datadoghq.eu`). +OAuth token and authorization URLs are derived from `auth.site`, so it must match your Datadog data center (e.g. `datadoghq.com`, `us5.datadoghq.com`, `datadoghq.eu`). If `auth.site` includes a custom subdomain (e.g. `myorg.us5.datadoghq.com`), the browser is sent to that subdomain for authorization, while the token exchange and upload requests still use the base site (`us5.datadoghq.com`). ### apps.identifier diff --git a/packages/plugins/apps/src/upload.test.ts b/packages/plugins/apps/src/upload.test.ts index 8fdf7a3e0..e637bfc3e 100644 --- a/packages/plugins/apps/src/upload.test.ts +++ b/packages/plugins/apps/src/upload.test.ts @@ -278,7 +278,7 @@ describe('Apps Plugin - upload', () => { ); }); - test('Should warn (not error) when app_builder_url is absent from the upload response', async () => { + test('Should build a fallback App Builder URL when app_builder_url is absent but app_builder_id is present', async () => { // Skip the release call entirely — this test only cares about the upload log. getDDEnvValueMock.mockImplementation((key) => key === 'APPS_PUBLISH' ? 'false' : undefined, @@ -291,45 +291,70 @@ describe('Apps Plugin - upload', () => { const { errors, warnings } = await uploadArchive(archive, context, logger); - // The upload itself succeeded — a missing display URL must never fail the build. expect(errors).toHaveLength(0); + expect(warnings).toHaveLength(0); const uploadLog = mockLogFn.mock.calls.find(([message]) => message.startsWith('Your application is available at'), ); - expect(uploadLog).toBeUndefined(); - // But it also must not go silent — surfaced as a warning so it's visible in CI - // output (see handle-upload.ts, which logs and aggregates `warnings` without - // failing the build, unlike `errors`). Names the app by its display name (not - // context.identifier, which is an opaque hash — see identifier.ts) and includes - // the app_builder_id, so there's a concrete, unambiguous next step (find it in - // the apps list, disambiguated by ID if names collide), not just a generic warning. - expect(warnings).toHaveLength(1); - expect(warnings[0]).toContain('Could not resolve the App Builder URL'); - expect(warnings[0]).toContain(context.name); - expect(warnings[0]).toContain('builder123'); + expect(uploadLog?.[0]).toContain( + 'https://app.datadoghq.com/app-builder/apps/edit/builder123?viewMode=preview', + ); // Reset — other tests in this file rely on getDDEnvValueMock's default // (undefined) behavior and beforeEach doesn't reset this particular mock. getDDEnvValueMock.mockReset(); }); - test('Should omit the app ID suffix when app_builder_id is also absent', async () => { + test('Should build the fallback URL against the custom subdomain when auth.site has one', async () => { + getDDEnvValueMock.mockImplementation((key) => + key === 'APPS_PUBLISH' ? 'false' : undefined, + ); + doAuthenticatedRequestMock.mockResolvedValueOnce({ + version_id: 'v123', + application_id: 'app123', + app_builder_id: 'builder123', + } as any); + + const { warnings } = await uploadArchive( + archive, + { ...context, siteSubdomain: 'myorg' }, + logger, + ); + + expect(warnings).toHaveLength(0); + const uploadLog = mockLogFn.mock.calls.find(([message]) => + message.startsWith('Your application is available at'), + ); + expect(uploadLog?.[0]).toContain( + 'https://myorg.datadoghq.com/app-builder/apps/edit/builder123?viewMode=preview', + ); + + getDDEnvValueMock.mockReset(); + }); + + test('Should warn (not error) when both app_builder_url and app_builder_id are absent', async () => { getDDEnvValueMock.mockImplementation((key) => key === 'APPS_PUBLISH' ? 'false' : undefined, ); // Matches the backend's own no-op-path edge case (app-builder-code's - // TestAppBuilderURL_EmptyAppBuilderID) — both app_builder_id and - // app_builder_url absent, not just the URL. + // TestAppBuilderURL_EmptyAppBuilderID) — with no ID, there's nothing to build a + // fallback link from, so this is the one case that still can't be resolved. doAuthenticatedRequestMock.mockResolvedValueOnce({ version_id: 'v123', application_id: 'app123', } as any); - const { warnings } = await uploadArchive(archive, context, logger); + const { errors, warnings } = await uploadArchive(archive, context, logger); + // The upload itself succeeded — a missing display URL must never fail the build. + expect(errors).toHaveLength(0); + const uploadLog = mockLogFn.mock.calls.find(([message]) => + message.startsWith('Your application is available at'), + ); + expect(uploadLog).toBeUndefined(); expect(warnings).toHaveLength(1); + expect(warnings[0]).toContain('Could not resolve the App Builder URL'); expect(warnings[0]).toContain(context.name); - expect(warnings[0]).not.toContain('app ID'); getDDEnvValueMock.mockReset(); }); @@ -431,7 +456,59 @@ describe('Apps Plugin - upload', () => { expect(releaseLog?.[0]).not.toContain('?viewMode'); }); - test('Should warn (not error) when app_builder_url is absent from the release response', async () => { + test('Should display app_builder_url verbatim even when auth.site has a custom subdomain configured', async () => { + doAuthenticatedRequestMock + .mockResolvedValueOnce({ + version_id: 'v123', + application_id: 'app123', + app_builder_id: 'builder123', + app_builder_url: + 'https://app.datadoghq.com/app-builder/apps/edit/builder123?viewMode=preview', + }) + .mockResolvedValueOnce({ + app_builder_url: 'https://app.datadoghq.com/app-builder/apps/builder123', + }); + + // The backend-provided app_builder_url is authoritative and always wins, even + // over our own subdomain config — it reflects whatever the backend actually + // resolved for this org, so it's shown as-is, never rewritten. + await uploadArchive(archive, { ...context, siteSubdomain: 'myorg' }, logger); + + const uploadLog = mockLogFn.mock.calls.find(([message]) => + message.startsWith('Your application is available at'), + ); + expect(uploadLog?.[0]).toContain( + 'https://app.datadoghq.com/app-builder/apps/edit/builder123?viewMode=preview', + ); + + const releaseLog = mockLogFn.mock.calls.find(([message]) => + message.startsWith('Published uploaded version'), + ); + expect(releaseLog?.[0]).toContain( + 'https://app.datadoghq.com/app-builder/apps/builder123', + ); + }); + + test('Should leave app_builder_url unchanged when it does not match the base site host', async () => { + doAuthenticatedRequestMock.mockResolvedValueOnce({ + version_id: 'v123', + application_id: 'app123', + app_builder_id: 'builder123', + app_builder_url: + 'https://dd.datad0g.com/app-builder/apps/edit/builder123?viewMode=preview', + }); + + await uploadArchive(archive, { ...context, siteSubdomain: 'myorg' }, logger); + + expect(mockLogFn).toHaveBeenCalledWith( + expect.stringContaining( + 'https://dd.datad0g.com/app-builder/apps/edit/builder123?viewMode=preview', + ), + 'info', + ); + }); + + test('Should build a fallback release URL against the custom subdomain when app_builder_url is absent from the release response', async () => { doAuthenticatedRequestMock .mockResolvedValueOnce({ version_id: 'v123', @@ -443,9 +520,60 @@ describe('Apps Plugin - upload', () => { // The backend couldn't resolve the org's app URL for this release — a real, // designed-for degradation path (e.g. a transient org-lookup failure), not a // hypothetical. The release itself still succeeded. app_builder_id is still - // present, though — it's set from the DB independently of the URL lookup. + // present, though — it's set from the DB independently of the URL lookup, so + // a fallback URL is constructed from it instead of warning. .mockResolvedValueOnce({ app_builder_id: 'builder123' }); + const { errors, warnings } = await uploadArchive( + archive, + { ...context, siteSubdomain: 'myorg' }, + logger, + ); + + // The release itself succeeded — a missing display URL must never fail the build. + expect(errors).toHaveLength(0); + expect(warnings).toHaveLength(0); + const releaseLog = mockLogFn.mock.calls.find(([message]) => + message.startsWith('Published uploaded version'), + ); + expect(releaseLog?.[0]).toContain( + 'https://myorg.datadoghq.com/app-builder/apps/builder123', + ); + }); + + test('Should reuse the upload response app_builder_id for the release fallback URL when the release response omits it', async () => { + doAuthenticatedRequestMock + .mockResolvedValueOnce({ + version_id: 'v123', + application_id: 'app123', + app_builder_id: 'builder123', + app_builder_url: + 'https://app.datadoghq.com/app-builder/apps/edit/builder123?viewMode=preview', + }) + // The release response identifies the same app the upload response already + // resolved an ID for, but doesn't repeat app_builder_id itself. + .mockResolvedValueOnce({}); + + const { errors, warnings } = await uploadArchive(archive, context, logger); + + expect(errors).toHaveLength(0); + expect(warnings).toHaveLength(0); + const releaseLog = mockLogFn.mock.calls.find(([message]) => + message.startsWith('Published uploaded version'), + ); + expect(releaseLog?.[0]).toContain( + 'https://app.datadoghq.com/app-builder/apps/builder123', + ); + }); + + test('Should warn (not error) when app_builder_id is absent from both the upload and release responses', async () => { + doAuthenticatedRequestMock + .mockResolvedValueOnce({ + version_id: 'v123', + application_id: 'app123', + }) + .mockResolvedValueOnce({}); + const { errors, warnings } = await uploadArchive(archive, context, logger); // The release itself succeeded — a missing display URL must never fail the build. @@ -457,11 +585,10 @@ describe('Apps Plugin - upload', () => { expect(releaseLog?.[0]).toContain('to live.'); expect(releaseLog?.[0]).not.toContain('\n'); // Surfaced as a warning rather than a blank/malformed log line — names the app - // by its display name and includes the app_builder_id for unambiguous lookup. - expect(warnings).toHaveLength(1); - expect(warnings[0]).toContain('Could not resolve the App Builder URL'); - expect(warnings[0]).toContain(context.name); - expect(warnings[0]).toContain('builder123'); + // by its display name for unambiguous lookup. + expect(warnings).toHaveLength(2); + expect(warnings[1]).toContain('Could not resolve the App Builder URL'); + expect(warnings[1]).toContain(context.name); }); test.each(['false', '0', 'False', 'FALSE', 'off', 'no'])( diff --git a/packages/plugins/apps/src/upload.ts b/packages/plugins/apps/src/upload.ts index eeef6cd3c..93238e8c2 100644 --- a/packages/plugins/apps/src/upload.ts +++ b/packages/plugins/apps/src/upload.ts @@ -18,7 +18,7 @@ import { APPS_API_PATH, ARCHIVE_FILENAME } from './constants'; type DataResponse = Awaited>; // Only the fields this file actually reads. app_builder_url is deliberately optional on both: -// see buildMissingAppUrlWarning's callers for what happens when the backend can't resolve it. +// see resolveAppBuilderUrl for what happens when the backend can't resolve it. type UploadApiResponse = { app_builder_id?: string; app_builder_url?: string; @@ -37,6 +37,7 @@ export type UploadContext = { identifier: string; name: string; site: string; + siteSubdomain?: string; version: string; }; @@ -54,17 +55,39 @@ export const getReleaseUrl = (site: string, appId: string) => { return `https://api.${site}/${APPS_API_PATH}/${appId}/release/live`; }; -// Builds the warning shown when the backend couldn't resolve an org's App Builder URL. -// Names the app (context.name — always human-readable, unlike context.identifier's opaque -// hash) and its app_builder_id when present, which disambiguates same-named apps and lets -// anyone who knows their org's domain construct the link directly. -const buildMissingAppUrlWarning = ( +// The backend-provided app_builder_url is authoritative and always wins, even over our own +// subdomain config, since it reflects whatever the backend actually resolved for this org. +// Only when the backend can't resolve one do we build our own from site + subdomain (falling +// back to the generic "app" host) and the app_builder_id — this can only ever be a guess at +// the backend's URL scheme, so it's a fallback, not a replacement for the backend's own value. +const resolveAppBuilderUrl = ( action: 'upload' | 'release', - name: string, - appBuilderId?: string, -) => { - const idSuffix = appBuilderId ? ` (app ID: ${appBuilderId})` : ''; - return `Could not resolve the App Builder URL for this ${action} — find "${name}"${idSuffix} in your App Builder apps list to view it.`; + site: string, + subdomain: string | undefined, + appBuilderUrl: string | undefined, + appBuilderId: string | undefined, +): string | undefined => { + if (appBuilderUrl) { + return appBuilderUrl; + } + + if (!appBuilderId) { + return undefined; + } + + const path = + action === 'upload' + ? `/app-builder/apps/edit/${appBuilderId}?viewMode=preview` + : `/app-builder/apps/${appBuilderId}`; + return `https://${subdomain ?? 'app'}.${site}${path}`; +}; + +// Builds the warning shown when the backend couldn't resolve an org's App Builder URL and we +// don't even have an app_builder_id to construct a fallback link from. Names the app +// (context.name — always human-readable, unlike context.identifier's opaque hash) so users can +// find it in their App Builder apps list. +const buildMissingAppUrlWarning = (action: 'upload' | 'release', name: string) => { + return `Could not resolve the App Builder URL for this ${action} — find "${name}" in your App Builder apps list to view it.`; }; export const getData = @@ -147,19 +170,20 @@ Would have uploaded ${summary}`, log.debug(`Uploaded ${summary}\n`); - if (response.app_builder_url) { - log.info(`Your application is available at:\n ${cyan(response.app_builder_url)}`); + const appBuilderUrl = resolveAppBuilderUrl( + 'upload', + context.site, + context.siteSubdomain, + response.app_builder_url, + response.app_builder_id, + ); + if (appBuilderUrl) { + log.info(`Your application is available at:\n ${cyan(appBuilderUrl)}`); } else { - // The backend couldn't resolve this org's App Builder URL (e.g. a transient - // lookup failure) — the upload itself still succeeded, so this doesn't fail the - // build. Uses context.name, not context.identifier, since the latter is an - // opaque hash in the default case (see identifier.ts's buildIdentifier) — not - // something anyone would recognize or search for. - const message = buildMissingAppUrlWarning( - 'upload', - context.name, - response.app_builder_id, - ); + // The backend couldn't resolve this org's App Builder URL and didn't even return + // an app_builder_id to build a fallback link from (e.g. a transient lookup + // failure) — the upload itself still succeeded, so this doesn't fail the build. + const message = buildMissingAppUrlWarning('upload', context.name); warnings.push(message); log.warn(message); } @@ -188,17 +212,22 @@ Would have uploaded ${summary}`, }, }); - if (releaseResponse.app_builder_url) { + const releaseAppBuilderUrl = resolveAppBuilderUrl( + 'release', + context.site, + context.siteSubdomain, + releaseResponse.app_builder_url, + // The release response can omit app_builder_id even though it identifies the + // same app the upload response already resolved one for. + releaseResponse.app_builder_id ?? response.app_builder_id, + ); + if (releaseAppBuilderUrl) { log.info( - `Published uploaded version ${bold(response.version_id)} to live.\n ${cyan(releaseResponse.app_builder_url)}`, + `Published uploaded version ${bold(response.version_id)} to live.\n ${cyan(releaseAppBuilderUrl)}`, ); } else { log.info(`Published uploaded version ${bold(response.version_id)} to live.`); - const message = buildMissingAppUrlWarning( - 'release', - context.name, - releaseResponse.app_builder_id, - ); + const message = buildMissingAppUrlWarning('release', context.name); warnings.push(message); log.warn(message); } diff --git a/packages/plugins/apps/src/vite/handle-upload.ts b/packages/plugins/apps/src/vite/handle-upload.ts index 8d5c75290..5e9d9558f 100644 --- a/packages/plugins/apps/src/vite/handle-upload.ts +++ b/packages/plugins/apps/src/vite/handle-upload.ts @@ -184,6 +184,7 @@ Either: identifier, name, site: auth.site, + siteSubdomain: auth.siteSubdomain, version, }, log,