Skip to content

fix: add missing appId warning to app build#486

Merged
rexxars merged 6 commits intomainfrom
sdk-970
Feb 27, 2026
Merged

fix: add missing appId warning to app build#486
rexxars merged 6 commits intomainfrom
sdk-970

Conversation

@bjoerge
Copy link
Member

@bjoerge bjoerge commented Feb 27, 2026

Description

Two issues fixed:

  1. buildApp was missing the warnAboutMissingAppId call that buildStudio had when auto-updates are enabled but no appId is configured.

  2. Both buildStudio and buildApp now skip the warning when called from
    deploy, since deploy handles appId itself (prompts the user to select/create an app and tells them to add it to config).

Added calledFromDeploy option to BuildOptions, set by deployStudio and deployApp so it can be ignored when build is invoked from deploy.

What to review

Makes sense?

Testing

Tests included

bjoerge and others added 3 commits February 27, 2026 15:34
Two issues fixed:
1. buildApp was missing the warnAboutMissingAppId call that buildStudio
   had when auto-updates are enabled but no appId is configured.
2. Both buildStudio and buildApp now skip the warning when called from
   deploy, since deploy handles appId itself (prompts the user to
   select/create an app and tells them to add it to config).

Added calledFromDeploy option to BuildOptions, set by deployStudio and
deployApp.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Verifies that:
- buildStudio/buildApp warn about missing appId when auto-updates enabled
- Warning is suppressed when calledFromDeploy is true
- Warning is suppressed when appId is configured

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@linear
Copy link

linear bot commented Feb 27, 2026

SDK-970 `warnAboutMissingAppId` fixes

9. warnAboutMissingAppId Missing from App Build

Original app build warns about missing appId when auto-updates enabled. New CLI omits this entirely.

10. warnAboutMissingAppId Always Shows in Studio Build

Original suppresses warning when called from deploy (since deploy has its own appId handling). New CLI always shows it.

@claude
Copy link

claude bot commented Feb 27, 2026

Claude finished @bjoerge's task in 2m 24s —— View job


Review

  • Read changed files
  • Analyze implementation
  • Post review feedback

The implementation is correct and the approach makes sense. One minor observation:

buildApp.ts warning placement (line 66 vs 73)

warnAboutMissingAppId is called at line 73, but getAutoUpdatesImportMap at line 66 already uses the (potentially undefined) appId. The warning would be marginally clearer if shown before the import map is configured — but this is cosmetic and has no functional impact since the build proceeds correctly regardless.

Test consistency note (buildStudio.appIdWarning.test.ts, line 104)

The autoUpdatesEnabled: true option passed to buildStudio in the tests is dead code — buildStudio ignores options.autoUpdatesEnabled and calls shouldAutoUpdate() internally (unlike buildApp which uses the option directly). This is a pre-existing design inconsistency in BuildOptions, not introduced by this PR. The tests work correctly via the shouldAutoUpdate mock, and the comment at line 135–136 explains it. Worth documenting in BuildOptions (e.g. a JSDoc noting the field is only honored by buildApp, not buildStudio) to avoid future confusion — but not blocking.

Everything else looks good: the calledFromDeploy guard is correctly applied in both buildApp and buildStudio, the flag is correctly set in deployApp and deployStudio, the omission of projectId from the buildApp warning call is intentional (apps use organizationId, not projectId), and the test coverage covers all four relevant cases.

@bjoerge bjoerge changed the title Sdk 970 fix: add missing appId warning to app build and suppress during deploy Feb 27, 2026
@bjoerge bjoerge changed the title fix: add missing appId warning to app build and suppress during deploy fix: add missing appId warning to app build Feb 27, 2026
… coverage

Remove the redundant warnAboutMissingAppId call in deployApp (the
"Next step" block already handles this), matching deployStudio's
approach. Add test cases for autoUpdatesEnabled: false in both
buildApp and buildStudio appId warning tests.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@github-actions
Copy link

github-actions bot commented Feb 27, 2026

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines 77.42% 7290 / 9415
🔵 Statements 76.99% 7511 / 9755
🔵 Functions 73.98% 1223 / 1653
🔵 Branches 64.35% 3393 / 5272
File Coverage
File Stmts Branches Functions Lines Uncovered Lines
Changed Files
packages/@sanity/cli/src/actions/build/buildApp.ts 94.93% 94.33% 100% 94.8% 45, 54-55, 105
packages/@sanity/cli/src/actions/build/buildStudio.ts 97.5% 96.07% 100% 97.46% 59, 126
packages/@sanity/cli/src/actions/deploy/deployApp.ts 84.21% 81.25% 100% 84.21% 38, 43, 88, 99-101, 145-146, 152
packages/@sanity/cli/src/actions/deploy/deployStudio.ts 94% 100% 100% 94% 32, 37, 130
Generated in workflow #2519 for commit 07ad2f8 by the Vitest Coverage Report Action

bjoerge and others added 2 commits February 27, 2026 16:24
…warning test

mockReturnValue persists across tests since clearAllMocks only resets
call tracking, not implementations. This caused the "appId configured"
test to pass for the wrong reason (auto-updates still disabled from
the previous test). Use mockReturnValueOnce so the override reverts
after a single call.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… test

Add comment explaining that buildStudio calls shouldAutoUpdate()
internally and ignores the autoUpdatesEnabled option, so the
mockReturnValueOnce on shouldAutoUpdate is what actually controls
the behavior in this test.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@bjoerge bjoerge marked this pull request as ready for review February 27, 2026 15:35
@bjoerge bjoerge requested a review from a team as a code owner February 27, 2026 15:35
@bjoerge bjoerge requested review from rexxars and removed request for a team February 27, 2026 15:35
@rexxars rexxars merged commit a05e45d into main Feb 27, 2026
33 checks passed
@rexxars rexxars deleted the sdk-970 branch February 27, 2026 16:42
@squiggler-legacy squiggler-legacy bot mentioned this pull request Feb 27, 2026
@squiggler-app squiggler-app bot mentioned this pull request Mar 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants