[APPS-2777] Fix: use app_builder_url from API response (fixes custom domain orgs)#449
Merged
gh-worker-dd-mergequeue-cf854d[bot] merged 5 commits intoJul 9, 2026
Conversation
14b7e06 to
8af9ab3
Compare
🎉 All green!🧪 All tests passed 🔗 Commit SHA: a9294fb | Docs | Datadog PR Page | Give us feedback! |
e9046ec to
de20efe
Compare
…ing it The backend now returns app_builder_url in upload and release responses, which reflects the correct host for orgs with custom domains (e.g. dd.datad0g.com). The previous hardcoded https://app.${site}/... produced wrong URLs for those orgs. The upload response's URL includes ?viewMode=preview (edit view); the release response's URL does not (published view) — this matches the backend behavior in ddoghq/dd-source#1038. Follows the "omit when absent" test discipline from #446: adds a test confirming no available-at message is logged when app_builder_url is absent from the upload response, matching the existing guard. Related: ddoghq/dd-source#1038
de20efe to
5b37ce7
Compare
Two gaps found during a production-readiness review of the backend fix (ddoghq/dd-source#12477): 1. The release path had no guard on app_builder_url at all — a missing value would interpolate into the log message as a blank line. 2. Both paths (upload already guarded, release didn't) went completely silent when the URL was missing, with no signal to the user or CI logs that anything was degraded. The backend's org lookup can legitimately fail (e.g. a transient ouisvc issue) — by design it doesn't fail the upload/release itself, so the frontend must handle "succeeded, but no URL" as a real, expected outcome rather than an edge case. Now both paths push a warning (surfaced via the existing warnings aggregation in handle-upload.ts, which logs but never fails the build) instead of going silent or printing malformed output.
The warning previously said "check the App Builder UI directly" with no concrete next step. Uses context.name -- not context.identifier, which is an opaque SHA-256 hash in the default case (see identifier.ts's buildIdentifier) and not something anyone would recognize or search for -- so the message tells the caller exactly what to search for in the App Builder apps list. This is deliberately still a warning, not a failure -- see PR discussion: failing the upload/release when ouisvc is down (even after retries) would couple app-builder-code's core CI/CD path to an unrelated service's availability, breaking every customer's automated pipeline over a convenience link they may never click. A missing URL costs one extra search in the App Builder UI; a failed upload costs a broken deploy.
app_builder_id is present whenever this warning fires, except for a narrow DB invariant violation in the backend's upload no-op path (documented in app-builder-code's service.go) that shouldn't occur in normal operation. Surfacing it alongside the display name gives an unambiguous reference: disambiguates if multiple apps share a name, and is directly usable by anyone who already knows their org's domain. Extracted the message construction into buildMissingAppUrlWarning to avoid duplicating the ID-suffix logic across the upload and release call sites. Added a test for the edge case where app_builder_id is also absent, confirming the suffix is omitted rather than printing something broken.
f9f7ab5 to
9ea97b4
Compare
| if (response.version_id && shouldPublish) { | ||
| const releaseUrl = getReleaseUrl(context.site, context.identifier); | ||
| await doAuthenticatedRequest({ | ||
| const releaseResponse: any = await doAuthenticatedRequest({ |
Contributor
Author
There was a problem hiding this comment.
Fair — fixed in a9294fb: both response and releaseResponse now use doAuthenticatedRequest()'s existing generic (matching vite/dev-server.ts's pattern) instead of any. Typecheck, lint, and all 26 unit tests still pass.
response/releaseResponse were typed any, so accessing app_builder_url/ app_builder_id/version_id had no compile-time protection against typos or backend field renames. Uses doAuthenticatedRequest<T>()'s existing generic, matching vite/dev-server.ts's pattern elsewhere in this package. Caught by an automated PR review (copilot-pull-request-reviewer) on #449.
sdkennedy2
approved these changes
Jul 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
https://app.${site}/..., which produces the wrong host for orgs with custom domains (e.g.dd.datad0g.com) — users on those orgs got a broken link printed after upload/release.app_builder_urldirectly in the upload and release API responses, resolved from the org's actual configured web domain (see backend fix below), so the plugin no longer needs to guess the domain.Changes
response.app_builder_url(server-provided, edit view +?viewMode=preview) instead of constructinghttps://app.${site}/app-builder/apps/${id}packages/plugins/apps/src/upload.tsreleaseResponse.app_builder_url(server-provided, published view, no query string) instead of a hardcoded message with no URLpackages/plugins/apps/src/upload.tsapp_builder_urlis missing from either response, surfaces awarningsentry instead of going silent (upload) or printing a blank line (release, which previously had no guard at all)packages/plugins/apps/src/upload.tshandle-upload.tswithout failing the buildpackages/plugins/apps/src/vite/handle-upload.tscontext.name), notcontext.identifier— the latter is a SHA-256 hash in the default case (seeidentifier.ts'sbuildIdentifier) and not something a human would recognize or search forpackages/plugins/apps/src/upload.tsapp_builder_idfor unambiguous lookup — disambiguates same-named apps, and is usable directly by anyone who already knows their org's domainpackages/plugins/apps/src/upload.ts?viewMode=previewpresent on upload / absent on release (default + custom-domain fixtures), warning text andapp_builder_idinclusion when the URL is missing, and the edge case whereapp_builder_idis also absent (mirrors the backend'sTestAppBuilderURL_EmptyAppBuilderID)packages/plugins/apps/src/upload.test.tsresponse/releaseResponsetyped viadoAuthenticatedRequest<T>()'s generic instead ofany, matchingvite/dev-server.ts's existing pattern — catches typos/renames onapp_builder_url/app_builder_id/version_idat compile time. Caught by automated PR reviewpackages/plugins/apps/src/upload.tsQA Instructions
Automated
CI on this PR: Unit tests ✅ VERIFIED, Linting ✅ VERIFIED, End to End ✅ VERIFIED.
Manual QA
This is a CLI/build-time plugin, not a hosted service, so there's no "test URL" to browse to. Three passes instead, each isolating a different part of the path — real plugin code vs. real network:
npm init @datadog/apps, local build linked indd-authcredentials)@datadog/vite-plugin, realvite buildnock)uploadArchive()function1. Real scaffold, fully live — the High Code Apps "Getting started" guide's documented flow, run for real: scaffold a project the way any customer would, link this branch's build into it instead of the npm-published version, and publish exactly as documented. Exact commands, in order (
npm 10.9.2,node v22.13.1):Steps 1-5 — scaffold, build, and link this branch's plugin in. The doc's own literal scaffolding syntax —
npm init @datadog/apps test-app --template vite-react -y— failed here: npm's owninit/-yflag handling mangles the args before they reachcreate-apps("Unknown Syntax Error: Command not found").npm create ... --forces npm to pass everything after--through untouched, which works — that's the one deviation from the doc's exact text below:Step 6 — edit
test-app/vite.config.ts. Not shell commands, so it's its own step: two changes, everything else (identifier, name,logLevel: 'debug'default, etc.) left exactly as scaffolded.Change 6a — point at staging instead of prod:
auth: { - site: 'datadoghq.com', + site: 'datad0g.com', apiKey: process.env.DD_API_KEY, appKey: process.env.DD_APP_KEY, },Change 6b — route traffic to this branch's test drive instead of shared staging. Add this block right after the imports at the top of the file, before the
hasDatadogApiKeysline. This is TD-only plumbing, not something a real app needs — a real app talking to real staging/prod skips this entirely:Steps 7-8 — publish, then clean up. Step 7 is the doc's own command, verbatim, no modification:
vite.config.tsships withlogLevel: 'debug'by default, which is also how the success line's exact wording got confirmed (see thelogLevelfinding in pass 2).2. Real plugin/build wiring, mocked network — done before pass 1's scaffold test existed, using a minimal hand-built fixture project instead of a real scaffold, with the network mocked (
nock) instead of a live test drive, specifically to isolate the missing-URL warning path in one run. Exact commands:Finding, not a regression: the success line (
Your application is available at: ...) useslog.info, and the plugin's own internal defaultlogLevelis'warn'(packages/factory/src/validate.ts) — with that bare default it never prints. In practice this rarely bites anyone:npm init @datadog/apps's generatedvite.config.tsalready setslogLevel: 'debug'explicitly (confirmed in pass 1), so every newly-scaffolded app shows this line unless someone removes that option. This predates APPS-2777 (confirmed viagit log -pon that line); it isn't something this PR introduced. See Out of Scope.3. Real backend integration via curl — the first verification done, before either build-driven pass above; full commands are in ddoghq/dd-source#12477's QA section. Kept here as the original data point:
Mutation-tested the fix by locally reintroducing the old bug (query-string leak, hardcoded domain) and confirming the relevant tests fail, then reverting and confirming they pass — see commit history for detail.
Blast Radius
@dd/apps-pluginpackage's post-upload/post-release console log message — no change to what gets uploaded, no change to request payloads, no change to error/warning handling.app_builder_urlreflectsapi.{site}rather than the org's real web domain — a regression from this plugin's pre-fix behavior for standard (non-custom-domain) orgs specifically. Should not merge/release ahead of #12477.Out of Scope / Follow-ups
packages/tests/src/e2e/appsPlugin/appsPlugin.spec.ts) don't includeapp_builder_url, so the E2E suite doesn't exercise the new log content — only unit tests doYour application is available at: ...) islog.info, silent under the plugin's own internal defaultlogLevel: 'warn'— pre-existing since APPS-2777, not introduced here, but discovered while QA'ing this PRlogLevel: 'info'for deployed apps, andnpm init @datadog/apps's generatedvite.config.tsalready ships withlogLevel: 'debug'by default (verified in Manual QA pass 1). The plugin's bare internal default only matters for hand-written configs that opt out of both. No code change proposedDocumentation
dd-auth --domain <site> -- npm run uploadflow this PR's console output feeds into, and thelogLevel: 'info'recommendation referenced above