feat: add workbench under unstable flags#907
Open
gu-stav wants to merge 82 commits into
Open
Conversation
|
Claude finished @gu-stav's task in 6m 1s —— View job Claude PR Review
A few issues worth fixing before this graduates from a tracking-only feature branch. 1. Typegen plugin is registered twice in dev mode (bug)In The existing tests use Fix: stop passing 2. Workbench requires authentication when only
|
Contributor
📦 Bundle Stats —
|
| Metric | Value | vs main (15636aa) |
|---|---|---|
| Internal (raw) | 2.7 KB | +539 B, +24.7% |
| Internal (gzip) | 1.0 KB | +257 B, +32.2% |
| Bundled (raw) | 11.15 MB | +19.2 KB, +0.2% |
| Bundled (gzip) | 2.10 MB | +5.3 KB, +0.2% |
| Import time | 867ms | -9ms, -1.0% |
bin:sanity
| Metric | Value | vs main (15636aa) |
|---|---|---|
| Internal (raw) | 782 B | - |
| Internal (gzip) | 423 B | - |
| Bundled (raw) | 9.87 MB | - |
| Bundled (gzip) | 1.77 MB | - |
| Import time | 1.96s | -80ms, -3.9% |
🗺️ 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 (15636aa3)
| Metric | Value | vs main (15636aa) |
|---|---|---|
| Internal (raw) | 106.7 KB | +8.5 KB, +8.7% |
| Internal (gzip) | 26.7 KB | +3.4 KB, +14.5% |
| Bundled (raw) | 21.71 MB | +7.8 KB, +0.0% |
| Bundled (gzip) | 3.45 MB | +3.5 KB, +0.1% |
| Import time | 769ms | -17ms, -2.2% |
🗺️ 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 (15636aa3)
| Metric | Value | vs main (15636aa) |
|---|---|---|
| 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.
Contributor
Coverage Delta
Comparing 71 changed files against main @ Overall Coverage
|
Contributor
Preview this PR with pkg.pr.newRun the Sanity CLInpx https://pkg.pr.new/sanity-io/cli/@sanity/cli@06ab142 <command>...Or upgrade project dependencies📦
|
joshuaellis
force-pushed
the
feat/workbench
branch
from
770d32d to
f7b42c8
Compare
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
Member
Author
|
The axios security warning will be resolved by module-federation/core#4644 I think |
Member
Author
|
Rebased onto |
joshuaellis
force-pushed
the
feat/workbench
branch
3 times, most recently
from
ffc8344 to
f2b1294
Compare
gu-stav
force-pushed
the
feat/workbench
branch
2 times, most recently
from
5e50a1d to
1e221bd
Compare
gu-stav
force-pushed
the
feat/workbench
branch
2 times, most recently
from
16e7dfe to
319ef83
Compare
joshuaellis
force-pushed
the
feat/workbench
branch
2 times, most recently
from
905b492 to
cf3839a
Compare
gu-stav
force-pushed
the
feat/workbench
branch
2 times, most recently
from
fcf69a8 to
d73d551
Compare
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
joshuaellis
force-pushed
the
feat/workbench
branch
3 times, most recently
from
1f4a287 to
1ca2304
Compare
…ion build path Rebasing onto main brought in the single-vite vendor build (#1223): buildVendorDependencies → resolveVendorBuildConfig, importMap → AutoUpdatesBuildConfig, a 2-arg finalizeViteConfig, and getViteConfig moving into @sanity/cli-build. Fold those into the workbench/federation build path: - getViteConfig: adopt the autoUpdates vendor-chunk model on the client path, keep the isWorkbenchApp federation plugin branch. - buildApp/buildStudio/buildStaticFiles: switch to resolveVendorBuildConfig + autoUpdates, keep the services/views/isWorkbenchApp passthrough. - tests: getViteConfig.test.ts moved to the autoUpdates API (federation cases kept); restore main's buildStaticFiles vendor integration tests and move the federation unit tests to buildStaticFiles.federation.test.ts.
…orkbench projects (#1257) * fix(dev): restore dashboard URL output for non-workbench apps The workbench dev server work replaced the dashboard URL announcement with a bare port log for every app. Non-federated apps still load through the dashboard, so dropping the URL left users with nothing to open. Bring back main's output for them and keep the port-only line for workbench apps, where the workbench URL is announced instead. * chore: update auto-generated changeset for PR #1257 * fix(dev): re-add --load-in-dashboard with main's semantics The flag was dropped wholesale on this branch, changing studio and app output relative to main. Restore main's behavior for non-workbench projects (defaults, warnings, dashboard URL flows) and ignore the flag for workbench apps — warning when it's passed explicitly, since they never load through the dashboard. Also consolidates the dev suites' duplicated test setup (output mock, flags, dev-server config, vite-result factory, fetch stub) into the shared testHelpers. * chore: update auto-generated changeset for PR #1257 * fix(dev): satisfy mockResolvedValue arity in shared dev test helper The pre-commit eslint --fix strips the explicit undefined argument, which tsc then rejects. Use a plain resolved-promise mock instead so the formatter has nothing to rewrite. --------- Co-authored-by: squiggler-app[bot] <265501495+squiggler-app[bot]@users.noreply.github.com>
* feat(workbench): deploy federated builds and log what ships to Brett `sanity deploy` required `index.html` in the build output, but federated (`unstable_defineApp`) builds emit only a module federation remote — every deploy died on a generic "Error checking directory" with the real cause hidden behind a debug flag. checkDir now validates the federation shape instead: the manifest must parse, declare a remote entry and at least one exposed module, and every referenced asset must exist on disk — catching partial or stale builds before upload. checkDir failures surface verbatim, and both deploy paths (studio and coreApp) log the upload target, metadata, and tarball contents before posting to the user-applications service. * chore: update auto-generated changeset for PR #1243 * chore(deploy): remove upload summary logging Keeps the PR scoped to federated build validation. The endpoint and tarball listing printed on every deploy was debugging aid, not product output. * chore: update auto-generated changeset for PR #1243 * chore(deploy): cut federation validation back to manifest presence and exposes Checking the remote entry and every referenced asset against disk re-verified what the bundler already guarantees and would break on any manifest format change. An empty exposes list still fails: a federated app must export at least one interface. * refactor(deploy): split checkWorkbenchAppDir out of checkDir A boolean option made checkDir's contract depend on the app shape. Callers already know whether they deploy a workbench app, so each build shape gets its own self-contained check and checkDir keeps its original index.html contract. * chore(deploy): check declared interfaces instead of built exposes The interfaces a federated app ships are exactly what unstable_defineApp declares (entry, views, services), so the at-least-one rule reads the config and fails before any prompts or API calls instead of parsing mf-manifest.json after the build. The manifest check shrinks to an existence marker that a federation build ran. Workbench studios always expose the studio config, so the rule only applies to the app path. * chore(deploy): say views or services instead of interfaces in the no-interface error "Interfaces" is application-service vocabulary; views and services are the words the user typed into unstable_defineApp. * refactor(deploy): gather workbench deploy checks in workbenchChecks The config-level interface check and the build-dir check guard the same deploy, so they live in one module: checkWorkbenchApp (throws like its sibling) and checkWorkbenchAppDir. * chore(deploy): mention entry in the no-interface deploy error --------- Co-authored-by: squiggler-app[bot] <265501495+squiggler-app[bot]@users.noreply.github.com>
…ench projects (#1259) * fix(build): restore strict-port for plain studio dev servers The workbench refactor hardcoded strictPort: false, so a plain studio on a busy port silently drifted to the next free one instead of failing like on main. Only apps and workbench runs need the fallback. * fix(dev): keep default signal handling for plain projects devAction installed SIGINT/SIGTERM handlers for every project, changing Ctrl-C semantics (and exit codes) for plain runs that have no workbench lock or registry entry to clean up. Only register them when workbench state exists. * fix(dev): only override the port flag when the workbench runs devAction pre-resolved the port and injected it into flags for every project, taking resolution away from getDevServerConfig where main does it. The override only exists so the app server can move off the port the workbench claimed — without a workbench the flags now pass through untouched. * chore: update auto-generated changeset for PR #1259 * chore: update auto-generated changeset for PR #1259 --------- Co-authored-by: squiggler-app[bot] <265501495+squiggler-app[bot]@users.noreply.github.com>
…ver (#1262) Everything the workbench vite server serves is either plain JS (the generated workbench.js), compiled dist (sanity/workbench), or TypeScript source that vite's own esbuild transforms (@sanity/workbench). plugin-react excludes node_modules from fast refresh, so it never transformed a single module here — its only output was a preamble script nothing consumed. Removing it also drops @vitejs/plugin-react from @sanity/cli's dependencies, keeping vite plugins contained to @sanity/cli-build.
…/cli re-exports (#1269)
…#1267) * fix(dev): display localhost for non-routable workbench bind addresses sanity dev --host 0.0.0.0 printed http://0.0.0.0:3333 as the workbench URL — both for the binding process and for any process reusing the lock, which stores the bind host verbatim. 0.0.0.0/:: don't open in every browser (notably on Windows). Only the displayed URL is normalized; the bind address and lock contents are untouched. * chore: update auto-generated changeset for PR #1267 --------- Co-authored-by: squiggler-app[bot] <265501495+squiggler-app[bot]@users.noreply.github.com>
* feat(cli-build)!: require organizationId in unstable_defineApp unstable_defineApp is the workbench opt-in and the organization ID is what the workbench runs and deploys against, so it's part of the contract — required in the schema and the DefineAppInput/DefineAppResult types, with a pointed validation message. Ports sanity-io/workbench#250 to the vendored extension API. * chore: update auto-generated changeset for PR #1279 --------- Co-authored-by: squiggler-app[bot] <265501495+squiggler-app[bot]@users.noreply.github.com>
* fix(dev): release the workbench lock when server creation throws createWorkbenchViteServer can throw after the lock is acquired (runtime- file write failure, invalid remote URL); only the undefined-result path released it, so a throw leaked the lock file until the next acquire pruned the stale PID. * chore: update auto-generated changeset for PR #1266 --------- Co-authored-by: squiggler-app[bot] <265501495+squiggler-app[bot]@users.noreply.github.com>
Residue of the main sync after the rebase: the regenerated lockfile entries (sanity v6 fixture refs, @types/node 22 peer suffixes) and the organizationId brand on deploy.studio.test.ts's unstable_defineApp call required by #1279.
…rs (#1264) * fix(build): apply caller-provided vite plugins to workbench dev servers additionalPlugins (typegen in dev) was only spread into the non-workbench plugin branch, so typegen.enabled was silently ignored for workbench apps. The plugins aren't client-specific — hoist them out of the branch. * chore: update auto-generated changeset for PR #1264 --------- Co-authored-by: squiggler-app[bot] <265501495+squiggler-app[bot]@users.noreply.github.com>
…pp (#1265) unstable_defineApp is the workbench opt-in and carries the organization ID, so the fallback that resolved it from the configured project is gone — it needed an authenticated user (a logged-out sanity dev died on a raw auth error) and an API round-trip on every startup for something the opt-in already declares. A missing ID is now a readable error pointing at unstable_defineApp in sanity.cli.ts.
* ci: revert snapshot-release.yml to main The branch had gutted the forceBump full-release half of the workflow and hardcoded the workbench dist-tag; merging that to main would have silently removed the manual release capability. Restoring main's file verbatim removes the merge delta entirely. Branch snapshots are published by dispatching the workflow manually with 'workbench' as the tag input — pushes to feat/workbench no longer auto-publish. * chore: curate the workbench changesets for the main changelog 40 accumulated changesets — auto-generated per-PR entries, superseded alpha bumps, and snapshot-publish-ordering notes — would have landed in the public changelog verbatim at merge. Squashed into three human-written entries; the resulting bump plan is unchanged (majors across the board, driven by main's Node 20 drop), so branch snapshot releases version identically.
… entry The dts change ships as part of the workbench feature; the curated workbench.md entry already covers it for the public changelog.
…1287) * fix(build): fail fast when a workbench studio has no sanity config An explicit applicationType: 'studio' wins over detection, so the federation studio path is reachable without a sanity.config file. The non-null assertion let null flow into the generated remote entry, which then imports its config from "null" — a broken build with no hint at the cause. * chore: update auto-generated changeset for PR #1287 --------- Co-authored-by: squiggler-app[bot] <265501495+squiggler-app[bot]@users.noreply.github.com>
…1284) * fix(workbench): stop wiping studio interfaces from the dev registry The studio extract hardcoded interfaces: undefined while the registry patch is a shallow merge — the panels/workers forwarded by the initial registration were wiped on the first regeneration, which fires right at watcher startup. Re-derive them from a fresh config read, the way the core-app extract already does. * chore: update auto-generated changeset for PR #1284 --------- Co-authored-by: squiggler-app[bot] <265501495+squiggler-app[bot]@users.noreply.github.com>
debug is imported in getViteConfig.ts for type checking only; without the ignore knip flags @types/debug as unused once dependencies shift.
* revert(workbench): keep federated remote type generation off Reverts the behavior change from #1282. Real TS apps fail with TYPE-001: the exposes are generated .js/.jsx shims, which tsc refuses without allowJs (TS6504) — no app template sets it. With allowJs worked around, declaration emit pulls in the user's own noEmit app code, which was never written to be declaration-emittable (TS2742 non-portable inferred types under pnpm, TS4082 private names in default exports). The #1282 verification only passed because the federated-studio fixture set allowJs — removed here so the fixture matches real templates. The recursive plugin scoping from #1282 stays: it's correct regardless. * chore: update auto-generated changeset for PR #1288 --------- Co-authored-by: squiggler-app[bot] <265501495+squiggler-app[bot]@users.noreply.github.com>
…anges (#1286) * fix(workbench): rebuild federated studios when their interface set changes Studios declare views/services in unstable_defineApp like apps do, but the dev flow assumed they couldn't: no rebuild hook, and the manifest watcher only reacted to sanity.config.* (the resolved project root) while the declarations live in sanity.cli.*. Adding or removing a studio view/service required a manual dev-server restart to get an expose for it. * chore: update auto-generated changeset for PR #1286 --------- Co-authored-by: squiggler-app[bot] <265501495+squiggler-app[bot]@users.noreply.github.com>
#1306) Studio scaffolds were missing the required organizationId, so a generated studio config wouldn't validate. The rest shrinks the exposed surface and aligns with existing conventions — no other behavior change. - drop the unused 'system' application type (only 'studio' is ever branched on) - resolve the registry/ barrel into direct imports (barrels defeat tree-shaking on the CLI hot path) and inline the two-use invariant helper - viewDeployment -> zod/mini, getEntryModule entry -> string | undefined, checkWorkbenchApp -> checkCanDeployWorkbenchApp, restore the inline schemaExtraction field - comments lead with why over what; drop 3 tests that only assert internal wiring; drop per-PR workbench changesets already covered by workbench.md
… main main relocated `checkRequiredDependencies` into `@sanity/cli-build/_internal/build` (#1301); point the studio dev-server test mock at the new module and regenerate the lockfile against the rebased tree.
* refactor(workbench): extract into a removable @sanity/workbench-cli package Rebased onto feat/workbench. Because feat/workbench predates the deepen-modules refactors (#1307) this work builds on, this commit also carries those (fold lock into the registry, expand extension artifacts once, typed applicationType, etc.) as prerequisites. The extraction itself: workbench lives in @sanity/workbench-cli, reached through entries that mirror the commands it integrates with — . authoring API (unstable_define*) /build federation Vite plugins + the build accessor /deploy the deploy guards /dev the dev-server registry /init the sanity.cli.ts config templates Core (cli, cli-build) discriminates on the Symbol.for brand and reaches the package only through those seams, so the feature stays removable. * refactor(workbench): sharpen the @sanity/workbench-cli public surface /build and /deploy both exported `getWorkbench` for different shapes — /build aliased the bare resolver, /deploy added the deploy guards. Drop the alias so /build exports `resolveWorkbenchApp` under its real name and `getWorkbench` means one thing. Retag the build-internal artifact and federation types `@internal`; they carried `@public` but reach no entry, so the tag misrepresented the surface. * test(workbench): run @sanity/workbench-cli in the root vitest suite The extraction added the package with 12 test files the root `vitest run` never executed — it wasn't in `test.projects` (nor coverage `include`). Register it so the suite and coverage actually cover the package. * chore(workbench): reconcile lockfile after rebase onto feat/workbench * test(workbench): real-path coverage for dev, deploy + e2e fix (#1317) * fix(cli-e2e): pack @sanity/workbench-cli for the e2e binary The extraction made `@sanity/workbench-cli` a dependency of `@sanity/cli` and `@sanity/cli-build`, but it isn't published to npm — so installing the packed `@sanity/cli` tarball failed resolving it from the registry, breaking every e2e run at globalSetup. Pack and install it alongside the others. * test(cli-e2e): cover dev (studio, app, workbench) and workbench init dev had no e2e coverage. These run the real binary end-to-end: the studio and app servers serve over HTTP and shut down on Ctrl+C, and the federated-studio dev runs the module-federation pipeline unmocked (the in-process test stubs it). init gains a workbench case asserting the `unstable_defineApp` config is scaffolded. * test(workbench): exercise real module-federation composition The federation plugin tests mocked `@module-federation/vite` and only checked call args / the scoping wrapper — they'd pass even if the real plugin shape changed. Add a test that runs the real integration and asserts it composes named, environment-scoped vite plugins. Reclassify the pure option-shaping test as `.unit.test.ts` (and exclude unit tests from the package project, like @sanity/cli) so the taxonomy reflects what actually integrates. * test(deploy): assert the federation build satisfies the deploy gate deploy.studio.test.ts mocks the build away, so nothing verified that a real federation build produces output `checkBuiltOutput` accepts. Assert it against the real dist right after the federated build, and that a non-federation directory is rejected — closing the build→deploy seam. * test(cli-e2e): exercise the real workbench host in dev federated-studio pinned the stable `sanity` (no `sanity/workbench` export), so `sanity dev` fell back to the federation-build path and never started the workbench host. Pin the `sanity` workbench dist-tag so the runtime resolves, and assert the host-started message (host on the configured port, studio pushed to port+1) instead of the build-only manifest signal. * test(cli-e2e): cover workbench SDK app init Alongside the workbench studio case, scaffold an SDK app with `--unstable--workbench` and assert its `sanity.cli.ts` is the workbench app variant — `unstable_defineApp` with an `entry` (the navigable app view) and no studio config. * test(workbench): tidy federation-test casts and sharpen intent comments Replace three `eslint-disable` / `as any` casts in the federation real-composition test with a typed partial-`Environment` helper, and reword the build→deploy gate negative assertion to say why it matters (proving `checkBuiltOutput` isn't a no-op). * test(cli-e2e): guard that init/dev stay non-workbench without the flag The plain init/dev tests exercised the non-workbench path but asserted only fields the workbench variant also has, so a regression that enabled workbench without `--unstable--workbench` would have slipped by. Assert the brand (`unstable_defineApp`) is absent from default init configs, and that a plain project's `sanity dev` never logs the workbench host startup. * test(cli-e2e): pin the dev server port as the inverse workbench guard Replace the explicit `not.toContain('Workbench dev server started')` with a strict positive assertion: a plain project must report ready on the *configured* port. If the workbench host started by mistake it would push the server to port+1 (and suppress the studio's "running at" line), so the port-pinned `waitForText` fails on its own — no separate negative assertion needed. * docs(cli-e2e): drop misleading "shell" wording in the dev test header "Shell" is workbench-host terminology; these cases serve a plain studio/app.
![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 2 potential issues.
There are 3 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 369173f. Configure here.
| throw new Error( | ||
| 'Workbench requires an organization ID. Pass "organizationId" to unstable_defineApp() in sanity.cli.ts.', | ||
| ) | ||
| } |
There was a problem hiding this comment.
Workbench throws before app server can report missing org
Medium Severity
resolveOrganizationId in startWorkbenchDevServer throws unconditionally when organizationId is missing, but startAppDevServer has a graceful early-exit path for this case (returning {started: false, reason: 'missing-organization-id'}). Since startWorkbenchDevServer runs first in devAction, a branded workbench app that omits organizationId gets an unhandled throw instead of the user-friendly error message the app server path was designed to show.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 369173f. Configure here.
| if (workbenchAvailable || registration) { | ||
| process.once('SIGINT', onSignal) | ||
| process.once('SIGTERM', onSignal) | ||
| } |
There was a problem hiding this comment.
Signal handler uses once preventing graceful repeat termination
Low Severity
The onSignal handler is registered with process.once, which means it fires at most once per signal type. Since onSignal calls void close() (fire-and-forget), if cleanup is slow (e.g., a server takes time to close), the process appears unresponsive. A second SIGINT would use Node's default behavior (abrupt kill), bypassing any remaining cleanup. Using process.on with an internal guard and explicit process.exit() after cleanup would ensure deterministic shutdown.
Reviewed by Cursor Bugbot for commit 369173f. Configure here.
It must stay published — @sanity/cli and @sanity/cli-build depend on it at ^1.0.0 — so it can't be made private or added to the changesets `ignore` list (that hard-errors, demanding its non-private dependents be ignored too). Reduce its npm footprint instead: drop the keywords, mark the description and README as an internal, not-for-direct-use detail, and keep the architecture notes repo-only in ARCHITECTURE.md (excluded from the tarball by `files`). The publishable surface (name/version/files/exports) is unchanged.
snocorp
approved these changes
Jun 18, 2026
snocorp
left a comment
snocorp
left a comment
Contributor
There was a problem hiding this comment.
Some questions and minor suggestions but nothing blocking.
| import {schemaTypes} from './schemaTypes' | ||
|
|
||
| export default defineConfig({ | ||
| title: 'Basic Studio', |
Contributor
There was a problem hiding this comment.
Suggested change
| title: 'Basic Studio', | |
| title: 'Federated Studio', |
| reactStrictMode, | ||
| relativeConfigLocation, | ||
| }) | ||
| await fs.writeFile(path.join(runtimeDir, 'app.js'), appJsContent) |
Contributor
There was a problem hiding this comment.
I know this isn't new in this PR but does this cause the watcher to trigger unnecessarily?
|
|
||
| import {describe, expect, test} from 'vitest' | ||
|
|
||
| import {parseWorkbenchCliConfig} from '../workbenchApp' |
Contributor
There was a problem hiding this comment.
Suggested change
| import {parseWorkbenchCliConfig} from '../workbenchApp' | |
| import {parseWorkbenchCliConfig} from '../workbenchApp.js' |
| // which isn't published to npm — pack it too so the installed binary can | ||
| // resolve it from the co-installed tarball instead of hitting the registry. | ||
| console.log('Packing @sanity/workbench-cli...') | ||
| const workbenchCliTarball = packPackage('@sanity/workbench-cli') |
Contributor
There was a problem hiding this comment.
Will this still be done after the new package is published to npm?
| try { | ||
| const stats = await stat(sourceDir) | ||
| if (!stats.isDirectory()) { | ||
| throw new Error(`Directory ${sourceDir} is not a directory`) |
Contributor
There was a problem hiding this comment.
Kind of an awkward error message
| expect(content).toContain('<script type="module" src="./workbench.js">') | ||
| }) | ||
|
|
||
| test('is valid HTML with charset meta tag', async () => { |
Contributor
There was a problem hiding this comment.
Suggested change
| test('is valid HTML with charset meta tag', async () => { | |
| test('starts with a DOCTYPE and has a charset meta tag', async () => { |
|
|
||
| afterEach(() => { | ||
| vi.clearAllMocks() | ||
| vi.unstubAllEnvs() |
Contributor
There was a problem hiding this comment.
I don't think we need this line
| expect(manifest.startedAt).toBeDefined() | ||
| expect(manifest.version).toBe(1) | ||
|
|
||
| cleanup() |
Contributor
There was a problem hiding this comment.
Should this be in a finally clause?
| describe('watchRegistry', () => { | ||
| test('invokes callback when a manifest file is added', async () => { | ||
| const callback = vi.fn() | ||
| const watcher = watchRegistry(callback) |
Contributor
There was a problem hiding this comment.
Today I learned about using onTestFinished so you could add t to the params of this test callback and then use t.onTestFinished(() => watcher.close()) instead of closing the watcher at the end of the test logic to ensure it always gets called.
|
|
||
| afterEach(() => { | ||
| vi.clearAllMocks() | ||
| vi.unstubAllEnvs() |
Contributor
There was a problem hiding this comment.
I don't think we need this line
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
3 participants
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
Lands Workbench for the Sanity CLI: an opt-in way to develop and build federated applications (studios and SDK apps) behind a single dev server.
The shape.
sanity devfor an opted-in app starts an extra dev server — the workbench shell — and loads the real app into it over Module Federation. Same delivery model as Dashboard: one host, many apps mounted as remotes. The app's own dev server registers itself against that shell as a federated remote; the shell renders it.One server, many apps. Only one workbench runs per machine. The first opted-in
sanity devto start acquires a lockfile and hosts the shell; every othersanity devfinds it through the lock and registers its remote there. Run three apps locally and you get one workbench hosting three remotes.Opt-in is a single identity function. Apps opt in by wrapping their config in
unstable_defineApp(...)insanity.cli.ts. That's the only signal — it stamps a brand symbol the CLI detects (isWorkbenchApp). Without it, dev/build/deploy take the exact paths they take onmain. No flag, no env var, no behavioural change for anyone who doesn't call it.Build + deploy. A branded app builds a Module Federation remote (
mf-manifest.json, exposed./App/./views/*/./services/*) through asanity/federationVite plugin stack, instead of a plain SPA.sanity deployvalidates the federation manifest instead ofindex.html. Studios and SDK apps share the path.Scaffolding.
sanity init --unstable--workbenchscaffolds a federated app (wraps the config inunstable_defineApp). The flag is hidden and undocumented — internal-only while the API carries theunstable_prefix.Experimental; should not surface in release notes.
What to review
Three areas, in order of how much I'd want eyes on them.
1. Nothing changes without opting in. Every workbench branch is gated on
isWorkbenchApp()(the brand) or the internalSANITY_INTERNAL_IS_WORKBENCH_REMOTEenv var. Config parsing only forks after the brand check;strictPort, entry-module, Vite plugins and auto-updates all keepmain's logic on the non-workbench side; signal handlers attach only when there's a lock or registration to clean up;init --unstable--workbenchdefaults to off. The two newappfields (group,priority) are additive optionals.getCliConfig.tsandgetViteConfig.tssit on every command's hot path — worth a second read there.2. Lockfile + process handling. The single-workbench model rests on
workbenchLock.tsplus the registry.O_EXCL(wx) makes acquisition atomic, stale locks are pruned via liveness + OS start-time (PID reuse handled cross-platform, including Windows PowerShell), and the lock releases on normal exit / SIGINT / SIGTERM / throw-during-startup. Sharp edges to flag rather than block on:updatePort()writes the real Vite port back after the server listens, so there's a tiny window where a registrant reads the placeholder port — and that write isn't wrapped, so a failed write leaves a stale port silently.Testing
Coverage spans the new surface — Vite config, build artifacts, dev orchestration, the lock + registry (PID-reuse and Windows paths included), and deploy validation — plus a
federated-studiofixture. Non-workbench paths stay on the existing suites, unchanged and still green.Note
High Risk
Large changes on every-command hot paths (
getCliConfig,getViteConfig,devAction) plus multi-process dev (lock, registry, port stacking); mitigated by strictisWorkbenchAppgating but regressions would affect all CLI users if gating fails.Overview
Workbench is an opt-in local dev and build model: wrapping
appinunstable_defineApp(...)insanity.cli.tsbrands the config; the CLI treats that as the only workbench signal (isWorkbenchApp). Without it, behavior matchesmain.For branded projects,
sanity devis orchestrated indevAction: it may start a singleton workbench host on the configured port, runs the studio/app Vite server on the next port when the host is active, registers the remote (views/services/entry→ federation exposes), watches manifests, and rebuilds the app server when the declared interface set changes. The workbench remote itself usesSANITY_INTERNAL_IS_WORKBENCH_REMOTEto skip the shell and stay on the configured port.Build/deploy switch workbench apps to
@sanity/workbench-clifederation Vite plugins (skipping the normal SPA runtime/favicons path), support apps with noentry(stub runtime), relaxstrictPortfor stacked servers, and adjust deploy validation (federation output, view payload stub, block external deploy for federated studios).cli-core adds workbench config parsing (
parseWorkbenchCliConfig,applicationTypedetection), shared manifest schemas,getCliConfigUncachedfor live reloads, and sharedgetSanityConfigDir/ data dir helpers.Also adds
sanity init --unstable--workbench, afederated-studiofixture, packs@sanity/workbench-cliin e2e, exportsunstable_defineApp, and broad unit/e2e coverage so plain studios/apps still bind their configured dev port.Reviewed by Cursor Bugbot for commit 06ab142. Bugbot is set up for automated code reviews on this repo. Configure here.