Integrate Project model into loader and decompose into composable stages#7023
Integrate Project model into loader and decompose into composable stages#7023ryancbahan wants to merge 21 commits intomainfrom
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Coverage report
Test suite run success3921 tests passing in 1504 suites. Report generated by 🧪jest coverage report action from 1e6e6de |
6c3198a to
c7fcd8b
Compare
d7d2097 to
ff9497f
Compare
c7fcd8b to
d60ce08
Compare
288c896 to
b17ef80
Compare
|
We detected some changes at Caution DO NOT create changesets for features which you do not wish to be included in the public changelog of the next CLI release. |
b17ef80 to
57e86f9
Compare
d60ce08 to
2edfd38
Compare
18f3516 to
839512e
Compare
509aacf to
8964add
Compare
839512e to
87728e2
Compare
Differences in type declarationsWe detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:
New type declarationsWe found no new type declarations in this PR Existing type declarationspackages/cli-kit/dist/public/node/themes/api.d.ts@@ -5,7 +5,6 @@ export type ThemeParams = Partial<Pick<Theme, 'name' | 'role' | 'processing' | '
export type AssetParams = Pick<ThemeAsset, 'key'> & Partial<Pick<ThemeAsset, 'value' | 'attachment'>>;
export declare function fetchTheme(id: number, session: AdminSession): Promise<Theme | undefined>;
export declare function fetchThemes(session: AdminSession): Promise<Theme[]>;
-export declare function findDevelopmentThemeByName(name: string, session: AdminSession): Promise<Theme | undefined>;
export declare function themeCreate(params: ThemeParams, session: AdminSession): Promise<Theme | undefined>;
export declare function fetchThemeAssets(id: number, filenames: Key[], session: AdminSession): Promise<ThemeAsset[]>;
export declare function deleteThemeAssets(id: number, filenames: Key[], session: AdminSession): Promise<Result[]>;
packages/cli-kit/dist/public/node/themes/theme-manager.d.ts@@ -8,8 +8,8 @@ export declare abstract class ThemeManager {
protected abstract removeTheme(): void;
protected abstract context: string;
constructor(adminSession: AdminSession);
- findOrCreate(name?: string, role?: Role): Promise<Theme>;
- fetch(name?: string, role?: Role): Promise<Theme | undefined>;
+ findOrCreate(): Promise<Theme>;
+ fetch(): Promise<Theme | undefined>;
generateThemeName(context: string): string;
create(themeRole?: Role, themeName?: string): Promise<Theme>;
}
\ No newline at end of file
|
87728e2 to
490c047
Compare
8964add to
2929234
Compare
Wires the Project domain model into the existing loading pipeline: - getAppConfigurationState uses Project.load() for filesystem discovery - getAppConfigurationContext returns project + activeConfig + state as independent values (project is never nested inside state) - AppLoader reads from Project's pre-loaded data: extension files, web files, dotenv, hidden config, deps, package manager, workspaces - No duplicate filesystem scanning — Project discovers once, loader reads from it - AppConfigurationState no longer carries project as a field - LoadedAppContextOutput exposes project and activeConfig as top-level fields for commands - All extension/web file discovery filtered to active config's directories via config-selection functions Zero behavioral changes. All 3801 existing tests pass. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace the monolithic loadApp pipeline with composable stages:
- loadApp is now a thin wrapper: getAppConfigurationContext → loadAppFromContext
- loadAppFromContext takes narrow Project + ActiveConfig directly
- getAppConfigurationContext is discovery-only (no parsing/state construction)
- ReloadState replaces passing entire AppLinkedInterface through reloads
- AppLoader takes reloadState? instead of previousApp?
- link() returns {remoteApp, configFileName, configuration} (no state)
- linkedAppContext uses activeConfig directly, no AppConfigurationState
Remove dead code: AppConfigurationState, toAppConfigurationState,
loadAppConfigurationFromState, loadAppUsingConfigurationState,
loadAppConfiguration, getAppConfigurationState, getAppDirectory,
loadDotEnv, loadHiddenConfig, findWebConfigPaths, loadWebsForAppCreation.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…Path - Remove orphaned <<<<<<< HEAD marker and dead loadHiddenConfig tests - Re-add isAppConfigSpecification import (used at line 776) - Add missing configPath field to loadedConfiguration in loadAppFromContext - Fix configuration.path references to use configPath (path extracted from config type) - Fix testApp() call signature in use.test.ts Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…path Same bug as the other two configuration.path references fixed in e4487d9 — configuration is Zod-parsed and has no path property. Without this fix, activeConfigFile always returns undefined, causing loadWebs() and createExtensionInstances() to load all project files instead of filtering by the active config. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
isaacroldan
left a comment
There was a problem hiding this comment.
Review assisted by pair-review
1. Remove dead code fallback in createExtensionInstances — since configPaths is derived from extensionFiles, the find is guaranteed to match. Simplified to iterate extensionFiles directly. 2. Extract getAppConfigurationFileName, getAppConfigurationShorthand, isValidFormatAppConfigurationFileName, and AppConfigurationFileName into a leaf utility module (config-file-naming.ts). This breaks the circular import: loader → active-config → use → loader. Loader re-exports for backward compatibility. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
isaacroldan
left a comment
There was a problem hiding this comment.
The reload behaviour has changed, let's fix that first 🙏
I think some extensive tophat of app dev is necessary before merging, even it appears to work, we need to check the manifest.json and the bundle uploaded to ensure it includes everything
Three new tests verifying that extensions added to disk after initial load are correctly discovered on reload: 1. Project.load re-scans filesystem — fresh glob finds new files 2. reloadApp finds new extensions — full reload path works 3. handleWatcherEvents produces Created event — the event handler correctly calls reloadApp and appDiff classifies the new extension as created Also fixed setupRealApp to include api_version in the function extension TOML (required by strict validation mode used during reload). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Two pre-existing bugs prevented `app dev` from handling extensions created mid-session: 1. Chokidar v3 silently ignores non-existent directories. Now the file watcher creates extension directories (with .gitkeep) before starting, so new extensions are detected even if the directory didn't exist at startup. 2. createManifest() only wrote manifest.json during CREATE, not UPDATE. Now it always writes the full manifest before filtering modules for the API payload, keeping the on-disk bundle current. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Use AppLinkedInterface cast for reloadApp/handleWatcherEvents calls - Import AbortController from cli-kit for correct type compatibility - Rename short identifier `e` to `ev` Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Pulling context from Slack -- we discovered the reload issue with manifest writes is already on main, not introduced in this pr. I've added more tests here and will tophat thoroughly + document. |
Tests for getAppConfigurationFileName, getAppConfigurationShorthand, and isValidFormatAppConfigurationFileName covering default values, valid formats, slugification, shorthand extraction, and edge cases. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… cast - Strip glob suffixes (e.g. extensions/**) before mkdir to avoid creating literal ** directories. Chokidar handles globs natively. - Remove .gitkeep creation — chokidar only needs the directory to exist. - Add clarifying comment on client_id cast in app-context.ts. - Add test verifying glob stripping behavior. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ata init - Remove `delete rawConfig.path` — in the new architecture, path lives on TomlFile.path and is never mixed into .content. - Collapse configurationLoadResultMetadata into a single const assignment with usesLinkedConfig: true, since loadAppFromContext is always the linked-app path. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Load extension specifications once at module level instead of 8 times per test. This is the heaviest operation in the integration tests (loads all specs from disk + parses Zod schemas). Reduces memory churn that was causing fsevents SIGABRT on macOS CI runners. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The mkdir call added to FileWatcher.start() throws EACCES when the app directory is unwritable (e.g. test fixtures using '/' as directory). This unhandled rejection cascaded into an fsevents SIGABRT on macOS CI. - Wrap mkdir in try/catch since it's non-fatal (chokidar handles missing directories gracefully) - Hoist loadLocalExtensionsSpecifications to module level in integration tests to reduce memory pressure (was called 8x) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Prevents real chokidar/fsevents watchers from being created during tests, which caused native addon crashes on macOS CI runners. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…lly) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Multi-config dev tests (multi-config-dev.spec.ts): - Verify -c flag loads the named config (staging TOML, correct scopes) - Verify default config is used when -c is omitted - Assert on info banner, scopes, and Config filename in App info tab Dev hot-reload tests (dev-hot-reload.spec.ts): - Editing app config TOML (scopes change) triggers full reload pipeline - Creating a new extension mid-dev is detected by the file watcher - Deleting an extension mid-dev is detected by the file watcher Key design decisions: - Use flow_trigger extensions (build mode 'none') to avoid theme-check, esbuild, and npm install overhead in e2e temp directories - Start dev without extensions, mutate filesystem mid-dev to avoid dev session API rejection on initial CREATE - Assert on watcher detection messages (Extension created/deleted) rather than API success for extension create/delete tests - App config TOML edit test verifies the full round-trip including API success (App config updated + Updated dev preview) - Clear SHOPIFY_FLAG_CLIENT_ID when using -c flag (mutually exclusive) - Use --skip-dependencies-installation to avoid npm install hangs
…rrentConfigPreference type
1. Remove unused _extensionDirectories parameter from createExtensionInstances.
Extension discovery goes through activeConfigFile + extensionFilesForConfig,
making the parameter dead code.
2. Narrow setCurrentConfigPreference to accept {client_id?: string} instead of
AppConfiguration. The function only reads client_id — full schema validation
is deferred to the next command that loads the app. This removes the unsafe
'as AppConfiguration' cast on raw TOML content and drops the dubious
'asserts configuration is CurrentAppConfiguration' return type.
…NT_ID from env Empty string is still passed to the child process, causing the tunnel setup to use an empty client_id and fail with a Cloudflare unmarshal error. Setting to undefined causes the spawn helper's 'value !== undefined' filter to exclude it from the child process environment entirely.
| vi.useFakeTimers() | ||
| // Prevent real chokidar/fsevents watchers from being created in tests. | ||
| // The hook only needs the AppEventWatcher as an EventEmitter, not real file watching. | ||
| vi.spyOn(AppEventWatcher.prototype, 'start').mockResolvedValue() |
There was a problem hiding this comment.
this was hogging up memory now that tests are sharded
|
Added a bunch more tests around hot reload, file watching, and config switching. Doing a bunch of local tophatting and good so far. |
dmerand
left a comment
There was a problem hiding this comment.
I just have a couple of extra comments.
| const webFiles = activeConfig ? webFilesForConfig(this.project, activeConfig) : this.project.webConfigFiles | ||
| const webTomlPaths = webFiles.map((file) => file.path) | ||
| const webs = await Promise.all( | ||
| webFiles.map((webFile) => loadSingleWeb(webFile.path, this.abortOrReport.bind(this), webFile.content)), |
There was a problem hiding this comment.
extensionFilesForConfig() / webFilesForConfig() intentionally reduce configured globs to simple path prefixes, so patterns like foo/*/extensions over-match. This PR is where that simplification becomes production loader behavior: loadConfigForAppCreation(), loadWebs(), and createExtensionInstances() now route through those helpers instead of the old direct glob discovery. The new integration tests cover rescans and reloads, but they do not protect nontrivial glob shapes. Can we either preserve true glob semantics here or add regression coverage for complex extension_directories and web_directories patterns before merging?
There was a problem hiding this comment.
I'll update to preserve original behavior and add test.
| } | ||
| ]'`) | ||
| vi.mocked(loadAppConfiguration).mockRejectedValue(error) | ||
| vi.mocked(getAppConfigurationContext).mockRejectedValue(error) |
There was a problem hiding this comment.
This test now only proves that discovery errors propagate out of use(). If the intended contract is still "don’t cache a config unless it is valid enough to be used as the active app config," then I think we need at least one fixture-based test with a parseable-but-schema-invalid TOML and an assertion that the cached preference is not updated.
If the new contract is intentionally "syntactically valid + has client_id is enough for use()," then I’d suggest updating the test name/comments to make that boundary explicit, because the old test shape implied a stronger guarantee.
There was a problem hiding this comment.
I'll make the test more explicit that syntactic validation is the aim here.
| const {activeConfig} = await getAppConfigurationContext(directory, configFileName) | ||
| setCurrentConfigPreference(activeConfig.file.content, {configFileName, directory}) |
There was a problem hiding this comment.
I see the intent here is to make app config use only switch the active TOML and defer full schema validation to later commands. I’m still worried about the UX regression relative to the old path: this command used to reject configs that were parseable TOML but no longer valid app config, whereas now it can cache them as the active selection as long as client_id is present. That shifts the failure away from the point where the user made the choice, which makes diagnosis harder.
If that behavior change is intentional, could we call it out explicitly in the command contract/tests? Otherwise I think we should keep a narrow validated load here before writing the cached preference.
…() contract explicit 1. Replace prefix-based path matching in extensionFilesForConfig() and webFilesForConfig() with real glob matching via matchGlob (minimatch). This preserves the same glob semantics as Project.load()'s discovery step, correctly handling patterns like 'plugins/*' and 'extensions/**'. Add regression tests for glob patterns in both extension and web dirs. 2. Add explicit test for use() validation contract: 'accepts syntactically valid config with client_id even if schema-incomplete'. This documents that use() intentionally defers full schema validation to the next command that loads the app — only TOML syntax + client_id presence are checked.
Extends the manifest write tests to cover the 'created' event type (new extension added while dev is running), verifying the full manifest includes both existing and newly created extensions.

WHY are these changes introduced?
PR #7022 introduced the
ProjectandActiveConfigdomain models, but nothing consumed them (the loader still ran its own filesystem discovery, constructedAppConfigurationStateas an intermediate, and threaded the entire previousAppInterfacethrough reloads). This PR wires the new models into the loading pipeline and uses that as leverage to decompose the monolithicloadAppinto composable stages with narrow interfaces.WHAT is this pull request doing?
Decomposes the loader into three explicit stages:
getAppConfigurationContext(dir, configName)→{project, activeConfig}— discovery only, no parsing or state constructionloadAppFromContext({project, activeConfig, specifications, …})→AppInterface— validation and assembly, for callers that already hold a ProjectloadApp({directory, configName, …})— thin wrapper composing the two above[Interim state] Replaces
previousAppwith narrowReloadState:Instead of threading the entire
AppLinkedInterfacethrough reloads (just to read 2 fields),reloadAppnow constructs aReloadStatewith onlyextensionDevUUIDs: Map<string, string>andpreviousDevURLs. This is a step change away from broad state passing/mutation, but needs more consideration on "permanent home" as we continue to decompose functionality.Updates all consumers:
linkedAppContextusesactiveConfig.isLinkedandactiveConfig.file.contentdirectly instead of going throughAppConfigurationStatelink()returns{remoteApp, configFileName, configuration}— dropsstatefrom its return typeuse()readsactiveConfig.file.contentinstead of callingloadAppConfigurationloadConfigForAppCreationusesactiveConfigandproject.directorydirectlyRemoves dead code (-400 lines):
AppConfigurationState,AppConfigurationStateBasics,toAppConfigurationState,loadAppConfigurationFromState,loadAppUsingConfigurationState,loadAppConfiguration,getAppConfigurationState,getAppDirectory,loadDotEnv,loadHiddenConfig,findWebConfigPaths,loadWebsForAppCreation,getConfigurationPath(de-exported)How to test your changes?
Measuring impact
Checklist
🤖 Generated with Claude Code