refactor(deploy): extract deploy target resolution and auto-update rules#1254
Open
gu-stav wants to merge 1 commit into
Open
refactor(deploy): extract deploy target resolution and auto-update rules#1254gu-stav wants to merge 1 commit into
gu-stav wants to merge 1 commit into
Conversation
Deploy target precedence (appId over appHost, --url over studioHost) and auto-update deprecation detection each lived inside interactive code that prompts, creates applications, and exits — impossible to reuse read-only. Extracting them into verdict-returning resolvers gives the rules one home so consumers can't drift, and makes them testable as data-in/data-out.
Contributor
📦 Bundle Stats —
|
| Metric | Value | vs main (fbcbace) |
|---|---|---|
| Internal (raw) | 2.1 KB | - |
| Internal (gzip) | 799 B | - |
| Bundled (raw) | 11.13 MB | - |
| Bundled (gzip) | 2.10 MB | - |
| Import time | 858ms | -13ms, -1.5% |
bin:sanity
| Metric | Value | vs main (fbcbace) |
|---|---|---|
| Internal (raw) | 1023 B | - |
| Internal (gzip) | 486 B | - |
| Bundled (raw) | 9.87 MB | - |
| Bundled (gzip) | 1.77 MB | - |
| Import time | 1.95s | -5ms, -0.3% |
🗺️ View treemap · Artifacts
Details
- Import time regressions over 10% are flagged with
⚠️ - Sizes shown as raw / gzip 🗜️. Internal bytes = own code only. Total bytes = with all dependencies. Import time = Node.js cold-start median.
📦 Bundle Stats — @sanity/cli-core
Compared against main (fbcbace2)
| Metric | Value | vs main (fbcbace) |
|---|---|---|
| Internal (raw) | 96.3 KB | - |
| Internal (gzip) | 22.7 KB | - |
| Bundled (raw) | 21.70 MB | - |
| Bundled (gzip) | 3.45 MB | - |
| Import time | 764ms | -6ms, -0.8% |
🗺️ View treemap · Artifacts
Details
- Import time regressions over 10% are flagged with
⚠️ - Sizes shown as raw / gzip 🗜️. Internal bytes = own code only. Total bytes = with all dependencies. Import time = Node.js cold-start median.
📦 Bundle Stats — create-sanity
Compared against main (fbcbace2)
| Metric | Value | vs main (fbcbace) |
|---|---|---|
| Internal (raw) | 908 B | - |
| Internal (gzip) | 483 B | - |
| Bundled (raw) | 931 B | - |
| Bundled (gzip) | 491 B | - |
| Import time | ❌ ChildProcess denied: node | - |
Details
- Import time regressions over 10% are flagged with
⚠️ - Sizes shown as raw / gzip 🗜️. Internal bytes = own code only. Total bytes = with all dependencies. Import time = Node.js cold-start median.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit faad45d. Configure here.
| reason: 'app-not-found', | ||
| type: 'invalid', | ||
| } | ||
| } |
There was a problem hiding this comment.
appId beaten by host validation
Medium Severity
For external studio deploys, resolveStudioDeployTarget normalizes and validates studioHost before it looks up deployment.appId. A stale or invalid studioHost can trigger invalid-host even when appId would resolve an existing application, which breaks the intended appId-over-host precedence.
Reviewed by Cursor Bugbot for commit faad45d. Configure here.
Contributor
Coverage Delta
Comparing 5 changed files against main @ Overall Coverage
|
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
1 participant
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.
Description
Deploy target rules (appId-over-appHost precedence,
--urloverstudioHost, external URL validation) and auto-update deprecation detection lived inside interactive code that prompts, creates applications, and exits the process. The rules couldn't be reused read-only, and were only testable through full command runs.This pulls them into two rule-owners with side-effect-free interfaces:
resolveDeployTarget.ts— returns a verdict (found/would-create/needs-input/invalid/blocked).findUserApplicationForStudio/ForAppbecome adapters that turn verdicts into prompts, creation, and exits. Transport errors throw; they're not verdicts.resolveAutoUpdates— returns{enabled, issue}.shouldAutoUpdatestays as the printing adapter, so existing callers don't move.No behavior change intended. This is groundwork for
sanity deploy --dry-run(follow-up PR), which consumes the same verdicts without side effects — the rules get one home so the two paths can't drift.One wording artifact kept on purpose: an unknown appId still reports
Error finding user application: Cannot find app with app ID ….What to review
defaultbranches at consumers, so adding a verdict later fails compilation everywhere it must be handled.Testing
Existing command-level deploy tests pass unchanged — they pin the prompts, creation flows, error messages, and exit codes this refactor must preserve. New unit tests cover
resolveAutoUpdates(precedence, conflict, deprecations).Note
Medium Risk
Touches studio/app deploy resolution paths and CLI exit/warning behavior; refactor-only with existing deploy tests as the safety net, but mistakes in verdict mapping could change who gets prompted or which host is used.
Overview
Extracts deploy target and auto-update decision logic into side-effect-free helpers so the same rules can power interactive deploy and a future
--dry-run, without duplicating precedence or validation.Deploy targets: New
resolveDeployTarget.tsexposesresolveStudioDeployTargetandresolveAppDeployTarget, returning typed verdicts (found,would-create,needs-input,invalid,blocked). Studio rules (--urloverstudioHost,appIdover host, external URL checks) move out ofdeployStudio’s localresolveAppHostand out of the oldfindUserApplication*flows.findUserApplicationForStudioandfindUserApplicationForAppnow call those resolvers and only handle spinners, prompts, creation, and exits.Auto-updates: New
resolveAutoUpdatesreturns{enabled, issue}for flag/config precedence, deprecations, and conflicts;shouldAutoUpdatebecomes a thin layer that prints warnings/errors fromissue. Unit tests coverresolveAutoUpdatesdirectly.Intended behavior for deploy prompts, creation, and error text is unchanged.
Reviewed by Cursor Bugbot for commit faad45d. Bugbot is set up for automated code reviews on this repo. Configure here.