Skip to content

refactor: rename manifest types and share cache dir#1008

Merged
gu-stav merged 2 commits intomainfrom
chore/upstream-cache-dir-and-manifest-rename
May 5, 2026
Merged

refactor: rename manifest types and share cache dir#1008
gu-stav merged 2 commits intomainfrom
chore/upstream-cache-dir-and-manifest-rename

Conversation

@gu-stav
Copy link
Copy Markdown
Member

@gu-stav gu-stav commented Apr 23, 2026

Description

Pure refactoring:

  • share the Vite cacheDir using a new SANITY_CACHE_DIR constant, because we will need it in a 3rd place to avoid drift
  • rename AppManifest to CoreAppManifest to be more in-line with Brett, where SDK apps have type: coreApp

This will reduce the overall changes, introduced in #997, between feat/workbench and main


Note

Low Risk
Low risk refactor that centralizes Vite cache directory configuration and renames/validates the core app manifest type; main risk is unintended type/schema mismatch affecting app deploy manifest payloads.

Overview
Refactors cache directory configuration by introducing SANITY_CACHE_DIR and updating Vite cacheDir usage (including vendor builds and tests) to derive from this shared path.

Renames and tightens manifest typing by replacing AppManifest with CoreAppManifest, adding a zod-backed coreAppManifestSchema, and updating deploy/manifest extraction and deployment API typing to use the new validated manifest payload.

Reviewed by Cursor Bugbot for commit db62592. Bugbot is set up for automated code reviews on this repo. Configure here.

@gu-stav gu-stav requested a review from a team as a code owner April 23, 2026 12:09
@gu-stav gu-stav requested review from binoy14, mttdnt and ryanbonial and removed request for a team and ryanbonial April 23, 2026 12:09
@claude
Copy link
Copy Markdown

claude Bot commented Apr 23, 2026

Claude finished @gu-stav's task in 2m 29s —— View job


Review

  • Gather context and read changed files
  • Review changes for issues
  • Post findings

1. Changeset should be removed

.changeset/upstream-cache-dir-and-manifest-rename.md is present but this is a pure internal refactor — no public API or runtime behavior changes for consumers. A patch bump for @sanity/cli is not warranted. (I was unable to push the deletion due to permissions — please remove manually.)


2. version field type widened to z.string()

packages/@sanity/cli/src/actions/manifest/types.ts:30

The original AppManifest interface constrained version to the literal '1'. The new zod schema uses z.string(), which accepts any string value. Change to z.literal('1') to preserve the original constraint:

// before
version: z.string(),

// after
version: z.literal('1'),

This is especially relevant now that extractAppManifest.ts calls coreAppManifestSchema.parse() — the schema should enforce the exact version value it expects.

Fix this →


3. Test assertion is tautological (minor)

packages/@sanity/cli/src/actions/build/__tests__/getViteConfig.test.ts:119

The test asserts cacheDir: \${SANITY_CACHE_DIR}/vite`using the same constant imported from production code. IfSANITY_CACHE_DIRwere accidentally set to a wrong value, both production and the test would use it and the test would still pass. The previous hardcoded'node_modules/.sanity/vite'` string provided independent verification. Not blocking, but worth considering.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 23, 2026

📦 Bundle Stats — @sanity/cli

Compared against main (37f95779)

@sanity/cli

Metric Value vs main (37f9577)
Internal (raw) 2.1 KB -
Internal (gzip) 799 B -
Bundled (raw) 10.94 MB -
Bundled (gzip) 2.05 MB -
Import time 750ms +4ms, +0.5%

bin:sanity

Metric Value vs main (37f9577)
Internal (raw) 975 B -
Internal (gzip) 460 B -
Bundled (raw) 9.84 MB -
Bundled (gzip) 1.77 MB -
Import time 1.80s +7ms, +0.4%

🗺️ 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 (37f95779)

Metric Value vs main (37f9577)
Internal (raw) 93.1 KB -
Internal (gzip) 21.9 KB -
Bundled (raw) 21.60 MB -
Bundled (gzip) 3.42 MB -
Import time 712ms +4ms, +0.6%

🗺️ 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 (37f95779)

Metric Value vs main (37f9577)
Internal (raw) 976 B -
Internal (gzip) 507 B -
Bundled (raw) 50.7 KB -
Bundled (gzip) 12.6 KB -
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.

export const coreAppManifestSchema = z.object({
icon: z.optional(z.string()),
title: z.optional(z.string()),
version: z.string(),
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Version field type widened from literal to string

Low Severity

The old AppManifest interface constrained version to the string literal '1', but the new coreAppManifestSchema uses z.string(), widening it to any string. This loses compile-time enforcement that version is exactly '1'. A z.literal('1') would preserve the original type safety while still using zod.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit ad3ca35. Configure here.

Comment thread packages/@sanity/cli/src/actions/manifest/types.ts
@gu-stav gu-stav requested a review from joshuaellis April 23, 2026 12:12
joshuaellis
joshuaellis previously approved these changes Apr 23, 2026
Copy link
Copy Markdown
Member

@joshuaellis joshuaellis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes sense. Lets se what the rest of the team thinks.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 23, 2026

Coverage Delta

File Statements
packages/@sanity/cli/src/actions/build/buildVendorDependencies.ts 67.4% (±0%)
packages/@sanity/cli/src/actions/build/getViteConfig.ts 100.0% (±0%)
packages/@sanity/cli/src/actions/deploy/deployApp.ts 80.5% (±0%)
packages/@sanity/cli/src/actions/manifest/extractAppManifest.ts 93.1% (±0%)
packages/@sanity/cli/src/actions/manifest/types.ts 100.0% (±0%)
packages/@sanity/cli/src/constants.ts 100.0% (new)
packages/@sanity/cli/src/services/userApplications.ts 92.8% (±0%)

Comparing 7 changed files against main @ 37f957796a99f7eff882d6ab0c36b25b4be39165

Overall Coverage

Metric Coverage
Statements 84.2% (±0%)
Branches 74.1% (±0%)
Functions 84.0% (±0%)
Lines 84.6% (±0%)

Copy link
Copy Markdown
Contributor

@binoy14 binoy14 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good besides that

'@sanity/cli': patch
---

Extract `SANITY_CACHE_DIR` constant for the shared Vite cache path and rename the internal `AppManifest` type to `CoreAppManifest` (with a zod schema) for consistency with the workbench payload.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the changeset necessary?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@claude remove the changeset

Copy link
Copy Markdown
Contributor

@squiggler-app squiggler-app Bot May 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude finished @gu-stav's task in 28s —— View job


Changeset removed

  • Gather context
  • Delete .changeset/upstream-cache-dir-and-manifest-rename.md
  • Commit and push

Deleted the changeset file — it's not needed for this pure internal refactor (no public API or behavior changes).

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 3 total unresolved issues (including 2 from previous reviews).

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit d1ea114. Configure here.

sourcemap: true,
},
cacheDir: 'node_modules/.sanity/vite',
cacheDir: `${SANITY_CACHE_DIR}/vite`,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test uses same constant as production code

Low Severity

The test assertion for cacheDir was changed from the hardcoded string 'node_modules/.sanity/vite' to use the same SANITY_CACHE_DIR constant that production code uses. This makes the assertion tautological for the constant's value — if SANITY_CACHE_DIR were accidentally set to an incorrect path, both the production code and the test would use the wrong value, and the test would still pass. The previous hardcoded string provided an independent verification of the expected path.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit d1ea114. Configure here.

@gu-stav gu-stav force-pushed the chore/upstream-cache-dir-and-manifest-rename branch from d1ea114 to d885116 Compare May 5, 2026 11:38
@gu-stav gu-stav merged commit fa32892 into main May 5, 2026
55 checks passed
@gu-stav gu-stav deleted the chore/upstream-cache-dir-and-manifest-rename branch May 5, 2026 12:31
@squiggler-app squiggler-app Bot mentioned this pull request May 5, 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