Skip to content

feat: allow selecting app/studio to undeploy#625

Open
rexxars wants to merge 9 commits intomainfrom
feat/undeploy-prompt-for-app
Open

feat: allow selecting app/studio to undeploy#625
rexxars wants to merge 9 commits intomainfrom
feat/undeploy-prompt-for-app

Conversation

@rexxars
Copy link
Member

@rexxars rexxars commented Mar 12, 2026

Description

Not urgent, new behavior.

Sort of annoying that if you deploy a studio/app and don't remember/see to put the appId into your CLI config, undeploy just… doesn't work. Now, if there's no appId specified, we allow the user to select from a list of apps. If it detects that you're in an app folder, it will give you app choices. If you're in a studio folder, it will give you studios.

Prompts before actually undeploying.

What to review

Do the flows make sense?

Flowchart:

sanity undeploy
  │
  ├─ determineIsApp(cliConfig)
  │  ├─ isApp = true   (has app config)
  │  └─ isApp = false  (studio)
  │
  ▼
  resolveApplication(cliConfig, isApp)
  │
  ├─── APP PATH (isApp = true) ──────────────────────────────────────────
  │    │
  │    ├─ getAppId(cliConfig) → has appId?
  │    │  │
  │    │  ├─ YES → getUserApplication({appId, isSdkApp: true})
  │    │  │        ├─ found    → return it ✓
  │    │  │        ├─ 404      → "does not exist" → exit
  │    │  │        └─ error    → fail with error message
  │    │  │
  │    │  └─ NO → is unattended?
  │    │         ├─ YES → "No application ID provided" → exit
  │    │         └─ NO  → promptForApp()
  │    │                  │
  │    │                  ├─ no organizationId? → info "Cannot list" → exit
  │    │                  ├─ API error?         → fail with error
  │    │                  ├─ empty list?        → info "No deployed apps" → exit
  │    │                  └─ has apps           → select prompt → return selected
  │
  ├─── STUDIO PATH (isApp = false) ──────────────────────────────────────
  │    │
  │    ├─ has studioHost or deployment.appId?
  │    │  │
  │    │  ├─ YES → need projectId
  │    │  │        ├─ no projectId → error(NO_PROJECT_ID)
  │    │  │        └─ has projectId → getUserApplication({appHost, appId, projectId})
  │    │  │                           ├─ found    → return it ✓
  │    │  │                           ├─ 404      → "does not exist" → exit
  │    │  │                           └─ error    → fail with error message
  │    │  │
  │    │  └─ NO → is unattended?
  │    │         ├─ YES → "No application ID or studio host" → exit
  │    │         └─ NO  → promptForStudio()
  │    │                  │
  │    │                  ├─ no projectId? → info "Cannot list" → exit
  │    │                  ├─ API error?    → fail with error
  │    │                  ├─ empty list?   → info "No deployed studios" → exit
  │    │                  └─ has studios   → select prompt → return selected
  │
  ▼
  userApplication resolved (or exit)
  │
  ├─ is unattended? → skip confirmation
  ├─ interactive?   → confirmUndeploy() → user says no? → exit
  │
  ▼
  deleteUserApplication()
  ├─ error → fail with error
  └─ success
     │
     ├─ "Studio/Application undeploy scheduled."
     │
     ├─ has deployment.appId? → "Remember to remove `deployment.appId`"
     └─ has app.id?           → "Remember to remove `app.id`"

Testing

Added new tests for the new behavior

@claude
Copy link

claude bot commented Mar 12, 2026

Claude finished @rexxars's task in 2m 31s —— View job


Code Review

  • Gather context
  • Read changed files
  • Analyze for issues
  • Post review

userApplications.ts line 59–61 — latent bug in getUserApplication else branch

When isSdkApp=false and neither appHost nor appId is provided, the function calls the list endpoint /projects/${projectId}/user-applications without any filter. The API returns an array, but the return type is UserApplication | null and the caller treats the result as a single object. This path is currently unreachable (all callers of getUserApplication with isSdkApp: false always provide at least appHost or appId), but it's a footgun. The else branch should either be removed or explicitly guard against this case.

Fix this →


userApplications.ts line 168 — organizationId! assertion is misleading

query: {appType: 'coreApp', organizationId: organizationId!},

The non-null assertion suppresses the TypeScript error but if the internal overload is called without organizationId, the query includes organizationId: undefined, which may serialize unexpectedly. Since the public overload already enforces organizationId: string, the internal implementation overload could just omit the organizationId from the query conditionally, or the implementation should be narrowed to match the public API. Low risk in practice but the ! is a misleading signal.


undeploy.ts line 68–77 — reminder message shown for app.id regardless of which config was actually used

After a successful undeploy, the reminder logic at line 69–77 checks cliConfig.deployment?.appId first, then cliConfig.app?.id. But if the user was in the interactive-selection path (no configured ID at all), neither condition is true and no reminder is shown — which is correct. However, if both deployment.appId and app.id are set, only the deployment.appId reminder is shown. This is fine. The issue is: the reminder says "to avoid errors when redeploying" but the user just interactively selected an app, so they may not have either config set. This is currently a no-op, but adding a reminder when no config is set (suggesting they configure the ID) could improve UX after an interactive undeploy.

Not a bug, but the coverage drop (−1.8%) is likely from the post-undeploy reminder path not being covered for the interactive selection case.

@github-actions
Copy link

github-actions bot commented Mar 12, 2026

Coverage Delta

File Statements
packages/@sanity/cli/src/commands/undeploy.ts 100.0% (±0%)
packages/@sanity/cli/src/services/userApplications.ts 98.1% (+ 5.3%)

Comparing 2 changed files against main @ 2f44698e5e319fc69d5266b00ac7f50026011c13

Overall Coverage

Metric Coverage
Statements 81.6% (+ 0.1%)
Branches 70.2% (+ 0.1%)
Functions 79.8% (+ 0.1%)
Lines 82.0% (+ 0.1%)

rexxars and others added 5 commits March 12, 2026 11:47
- Wrap deleteUserApplication in try/catch so spinner fails properly on error
- Add test for delete API failure (500 response)
- Add missing 'Remember to remove' assertion on prioritizes deployment.appId test

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Narrow `err` to `Error | string` in promptForApplication catch block
- Distinguish null (API error) from empty array in getUserApplications
- Fix misleading "not been assigned" message when configured host doesn't exist
- Add test for app listing API failure

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove explicit {exit: 1} from app listing error to default to 2,
  matching the studio path behavior
- Add test for studio listing API failure in interactive mode

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove double error handling in promptForApplication by delegating
directly to promptForApp/promptForStudio. Add try/catch in both prompt
methods for consistent error handling and fix variable naming bug.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@rexxars rexxars force-pushed the feat/undeploy-prompt-for-app branch from 4c751af to 7f352e7 Compare March 12, 2026 18:48
rexxars and others added 4 commits March 12, 2026 12:11
- Use spin.info() instead of spin.fail() for empty app/studio lists
  since an empty result is not an error
- Remove error swallowing in getUserApplications for coreApp so server
  errors propagate with their original message
- Fix implementation return type to match overloads (no more | null)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Use getAppId() for the "remember to remove" check so it also triggers
  for the deprecated app.id config path
- Remove redundant this.parse() call since init() already populates flags
- Add explicit assertions to the rejection-after-selection test
- Add test for deprecated app.id showing the reminder message

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Show the right config key in the reminder message depending on whether
  deployment.appId or the deprecated app.id is set
- Remove unreachable {default: 'true'} fallback in getUserApplication

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix type error from removing ternary in getUserApplication by using
  proper branching for appHost
- Remove dead isSdkApp URI fallback (appId is required by the type)
- Use spin.info() consistently for missing config (orgId, projectId)
  instead of spin.fail() + this.log()
- Add test for studio deployment.appId with missing projectId

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@rexxars rexxars marked this pull request as ready for review March 12, 2026 20:38
@rexxars rexxars requested a review from a team as a code owner March 12, 2026 20:38
@rexxars rexxars requested review from laurenashpole and removed request for a team March 12, 2026 20:38
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.

1 participant