From 44bf2d5cc4bbda57ef94b6516334cdb7c3005c89 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Wed, 1 Apr 2026 11:25:57 +0200 Subject: [PATCH 1/5] Add manifest generation to include_assets build step - Add generateManifest boolean to IncludeAssetsConfigSchema (default false) - Add anchor and groupBy fields to ConfigKeyEntrySchema for grouped manifest entries - Track copy operations in pathMap to resolve output-relative paths - Deduplicate source paths to prevent indexed suffixes on shared files - Use sequential copy loop to prevent findUniqueDestPath race conditions - resolveManifestPaths walks manifest tree using pathMap; non-path strings pass through unchanged - 33 tests passing Co-authored-by: Claude Code --- .../build/steps/include-assets-step.test.ts | 586 +++++++++++++++++- .../build/steps/include-assets-step.ts | 149 +++-- .../include-assets/copy-config-key-entry.ts | 111 +++- .../steps/include-assets/generate-manifest.ts | 222 +++++++ 4 files changed, 990 insertions(+), 78 deletions(-) create mode 100644 packages/app/src/cli/services/build/steps/include-assets/generate-manifest.ts diff --git a/packages/app/src/cli/services/build/steps/include-assets-step.test.ts b/packages/app/src/cli/services/build/steps/include-assets-step.test.ts index 858c3c71b19..07d2e9cf731 100644 --- a/packages/app/src/cli/services/build/steps/include-assets-step.test.ts +++ b/packages/app/src/cli/services/build/steps/include-assets-step.test.ts @@ -353,10 +353,10 @@ describe('executeIncludeAssetsStep', () => { // When await executeIncludeAssetsStep(step, contextWithNestedConfig) - // Then — all three tools paths resolved and copied - expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/test/extension/tools-a.js', '/test/output') - expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/test/extension/tools-b.js', '/test/output') - expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/test/extension/tools-c.js', '/test/output') + // Then — all three tools paths resolved and copied (file paths → copyFile) + expect(fs.copyFile).toHaveBeenCalledWith('/test/extension/tools-a.js', '/test/output/tools-a.js') + expect(fs.copyFile).toHaveBeenCalledWith('/test/extension/tools-b.js', '/test/output/tools-b.js') + expect(fs.copyFile).toHaveBeenCalledWith('/test/extension/tools-c.js', '/test/output/tools-c.js') }) test('skips silently when [] flatten key resolves to a non-array', async () => { @@ -582,4 +582,582 @@ describe('executeIncludeAssetsStep', () => { expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/test/extension/theme', '/test/output') }) }) + + describe('manifest generation', () => { + beforeEach(() => { + vi.mocked(fs.writeFile).mockResolvedValue() + vi.mocked(fs.mkdir).mockResolvedValue() + // Source files exist; destination paths don't yet (so findUniqueDestPath + // resolves on the first candidate without looping). Individual tests can + // override for specific scenarios. + vi.mocked(fs.fileExists).mockImplementation(async (p) => + typeof p === 'string' && p.startsWith('/test/extension'), + ) + vi.mocked(fs.copyFile).mockResolvedValue() + vi.mocked(fs.copyDirectoryContents).mockResolvedValue() + vi.mocked(fs.glob).mockResolvedValue([]) + }) + + test('writes manifest.json with a single configKey inclusion using anchor and groupBy', async () => { + // Given + const contextWithConfig = { + ...mockContext, + extension: { + ...mockExtension, + configuration: { + extensions: [ + { + targeting: [ + {target: 'admin.app.intent.link', tools: './tools.json', url: '/editor'}, + ], + }, + ], + }, + } as unknown as ExtensionInstance, + } + + + + const step: LifecycleStep = { + id: 'gen-manifest', + name: 'Generate Manifest', + type: 'include_assets', + config: { + generateManifest: true, + inclusions: [ + { + type: 'configKey', + key: 'extensions[].targeting[].tools', + anchor: 'extensions[].targeting[]', + groupBy: 'target', + }, + ], + }, + } + + // When + await executeIncludeAssetsStep(step, contextWithConfig) + + // Then + expect(fs.writeFile).toHaveBeenCalledOnce() + const writeFileCall = vi.mocked(fs.writeFile).mock.calls[0]! + expect(writeFileCall[0]).toBe('/test/output/manifest.json') + const manifestContent = JSON.parse(writeFileCall[1] as string) + expect(manifestContent).toEqual({ + 'admin.app.intent.link': { + tools: 'tools.json', + }, + }) + }) + + test('merges multiple inclusions per target when they share the same anchor and groupBy', async () => { + // Given + const contextWithConfig = { + ...mockContext, + extension: { + ...mockExtension, + configuration: { + extensions: [ + { + targeting: [ + { + target: 'admin.app.intent.link', + tools: './tools.json', + instructions: './instructions.md', + url: '/editor', + intents: [{type: 'application/email', action: 'open', schema: './email-schema.json'}], + }, + ], + }, + ], + }, + } as unknown as ExtensionInstance, + } + + + + const step: LifecycleStep = { + id: 'gen-manifest', + name: 'Generate Manifest', + type: 'include_assets', + config: { + generateManifest: true, + inclusions: [ + { + type: 'configKey', + key: 'extensions[].targeting[].tools', + anchor: 'extensions[].targeting[]', + groupBy: 'target', + }, + { + type: 'configKey', + key: 'extensions[].targeting[].instructions', + anchor: 'extensions[].targeting[]', + groupBy: 'target', + }, + { + type: 'configKey', + key: 'extensions[].targeting[].intents[].schema', + anchor: 'extensions[].targeting[]', + groupBy: 'target', + }, + ], + }, + } + + // When + await executeIncludeAssetsStep(step, contextWithConfig) + + // Then — url is NOT in the manifest because no inclusion references it + expect(fs.writeFile).toHaveBeenCalledOnce() + const writeFileCall = vi.mocked(fs.writeFile).mock.calls[0]! + const manifestContent = JSON.parse(writeFileCall[1] as string) + expect(manifestContent).toEqual({ + 'admin.app.intent.link': { + tools: 'tools.json', + instructions: 'instructions.md', + intents: [{schema: 'email-schema.json'}], + }, + }) + }) + + test('produces one manifest key per targeting entry when multiple entries exist', async () => { + // Given + const contextWithConfig = { + ...mockContext, + extension: { + ...mockExtension, + configuration: { + extensions: [ + { + targeting: [ + {target: 'admin.intent.link', tools: './tools-a.js', intents: [{schema: './schema1.json'}]}, + {target: 'admin.other.target', tools: './tools-b.js', intents: [{schema: './schema2.json'}]}, + ], + }, + ], + }, + } as unknown as ExtensionInstance, + } + + + + const step: LifecycleStep = { + id: 'gen-manifest', + name: 'Generate Manifest', + type: 'include_assets', + config: { + generateManifest: true, + inclusions: [ + { + type: 'configKey', + key: 'extensions[].targeting[].tools', + anchor: 'extensions[].targeting[]', + groupBy: 'target', + }, + { + type: 'configKey', + key: 'extensions[].targeting[].intents[].schema', + anchor: 'extensions[].targeting[]', + groupBy: 'target', + }, + ], + }, + } + + // When + await executeIncludeAssetsStep(step, contextWithConfig) + + // Then — two top-level keys, one per targeting entry + expect(fs.writeFile).toHaveBeenCalledOnce() + const writeFileCall = vi.mocked(fs.writeFile).mock.calls[0]! + const manifestContent = JSON.parse(writeFileCall[1] as string) + expect(manifestContent).toEqual({ + 'admin.intent.link': { + tools: 'tools-a.js', + intents: [{schema: 'schema1.json'}], + }, + 'admin.other.target': { + tools: 'tools-b.js', + intents: [{schema: 'schema2.json'}], + }, + }) + }) + + test('does NOT write manifest.json when generateManifest is false (default)', async () => { + // Given + const contextWithConfig = { + ...mockContext, + extension: { + ...mockExtension, + configuration: { + extensions: [ + { + targeting: [{target: 'admin.intent.link', tools: './tools.json'}], + }, + ], + }, + } as unknown as ExtensionInstance, + } + + vi.mocked(fs.fileExists).mockResolvedValue(false) + + // No generateManifest field — defaults to false + const step: LifecycleStep = { + id: 'gen-manifest', + name: 'Generate Manifest', + type: 'include_assets', + config: { + inclusions: [ + { + type: 'configKey', + key: 'extensions[].targeting[].tools', + anchor: 'extensions[].targeting[]', + groupBy: 'target', + }, + ], + }, + } + + // When + await executeIncludeAssetsStep(step, contextWithConfig) + + // Then + expect(fs.writeFile).not.toHaveBeenCalled() + }) + + test('does NOT write manifest.json when generateManifest is true but all inclusions are pattern/static', async () => { + // Given — pattern and static entries never contribute to the manifest + vi.mocked(fs.glob).mockResolvedValue(['/test/extension/public/logo.png']) + vi.mocked(fs.copyFile).mockResolvedValue() + vi.mocked(fs.mkdir).mockResolvedValue() + + const step: LifecycleStep = { + id: 'gen-manifest', + name: 'Generate Manifest', + type: 'include_assets', + config: { + generateManifest: true, + inclusions: [ + {type: 'pattern', baseDir: 'public', include: ['**/*']}, + ], + }, + } + + // When + await executeIncludeAssetsStep(step, mockContext) + + // Then — no configKey inclusions → no manifest written + expect(fs.writeFile).not.toHaveBeenCalled() + }) + + test('writes root-level manifest entry from non-anchored configKey inclusion', async () => { + // Given — configKey without anchor/groupBy contributes at manifest root + const contextWithConfig = { + ...mockContext, + extension: { + ...mockExtension, + configuration: {targeting: {tools: './tools.json', instructions: './instructions.md'}}, + } as unknown as ExtensionInstance, + } + + + + const step: LifecycleStep = { + id: 'gen-manifest', + name: 'Generate Manifest', + type: 'include_assets', + config: { + generateManifest: true, + inclusions: [ + {type: 'configKey', key: 'targeting.tools'}, + {type: 'configKey', key: 'targeting.instructions'}, + ], + }, + } + + // When + await executeIncludeAssetsStep(step, contextWithConfig) + + // Then — root-level keys use last path segment; values are output-relative paths + expect(fs.writeFile).toHaveBeenCalledOnce() + const manifestContent = JSON.parse(vi.mocked(fs.writeFile).mock.calls[0]![1] as string) + expect(manifestContent).toEqual({ + tools: 'tools.json', + instructions: 'instructions.md', + }) + }) + + test('logs a warning and treats inclusion as root-level when only anchor is set (no groupBy)', async () => { + // Given — inclusion has anchor but no groupBy + const contextWithConfig = { + ...mockContext, + extension: { + ...mockExtension, + configuration: {targeting: {tools: './tools.json'}}, + } as unknown as ExtensionInstance, + } + + vi.mocked(fs.fileExists).mockResolvedValue(false) + + const step: LifecycleStep = { + id: 'gen-manifest', + name: 'Generate Manifest', + type: 'include_assets', + config: { + generateManifest: true, + inclusions: [ + {type: 'configKey', key: 'targeting.tools', anchor: 'targeting'}, + ], + }, + } + + // When + await executeIncludeAssetsStep(step, contextWithConfig) + + // Then — warning logged, inclusion treated as root entry + expect(mockStdout.write).toHaveBeenCalledWith( + expect.stringContaining('anchor without groupBy (or vice versa)'), + ) + const manifestContent = JSON.parse(vi.mocked(fs.writeFile).mock.calls[0]![1] as string) + expect(manifestContent).toHaveProperty('tools') + }) + + test('writes an empty manifest when anchor resolves to a non-array value', async () => { + // Given — "extensions" is a plain string, not an array; the [] flatten marker + // returns undefined, so the anchor group is skipped and the manifest is empty + const contextWithConfig = { + ...mockContext, + extension: { + ...mockExtension, + configuration: { + extensions: 'not-an-array', + }, + } as unknown as ExtensionInstance, + } + + + + const step: LifecycleStep = { + id: 'gen-manifest', + name: 'Generate Manifest', + type: 'include_assets', + config: { + generateManifest: true, + inclusions: [ + { + type: 'configKey', + key: 'extensions[].targeting[].tools', + anchor: 'extensions[].targeting[]', + groupBy: 'target', + }, + ], + }, + } + + // When + await executeIncludeAssetsStep(step, contextWithConfig) + + // Then — the anchor group was skipped; manifest.json is written but contains no entries + expect(fs.writeFile).toHaveBeenCalledOnce() + const writeFileCall = vi.mocked(fs.writeFile).mock.calls[0]! + const manifestContent = JSON.parse(writeFileCall[1] as string) + expect(manifestContent).toEqual({}) + }) + + test('skips items whose groupBy field is not a string', async () => { + // Given — one entry has a numeric target, the other has a valid string target + const contextWithConfig = { + ...mockContext, + extension: { + ...mockExtension, + configuration: { + extensions: [ + { + targeting: [ + {target: 42, tools: './tools-bad.js'}, + {target: 'admin.link', tools: './tools-good.js'}, + ], + }, + ], + }, + } as unknown as ExtensionInstance, + } + + + + const step: LifecycleStep = { + id: 'gen-manifest', + name: 'Generate Manifest', + type: 'include_assets', + config: { + generateManifest: true, + inclusions: [ + { + type: 'configKey', + key: 'extensions[].targeting[].tools', + anchor: 'extensions[].targeting[]', + groupBy: 'target', + }, + ], + }, + } + + // When + await executeIncludeAssetsStep(step, contextWithConfig) + + // Then — only the string-keyed entry appears + expect(fs.writeFile).toHaveBeenCalledOnce() + const writeFileCall = vi.mocked(fs.writeFile).mock.calls[0]! + const manifestContent = JSON.parse(writeFileCall[1] as string) + expect(manifestContent).toEqual({ + 'admin.link': {tools: 'tools-good.js'}, + }) + expect(manifestContent).not.toHaveProperty('42') + }) + + test('writes manifest.json to outputDir derived from extension.outputPath', async () => { + // Given — outputPath is a file, so outputDir is its dirname (/test/output) + const contextWithConfig = { + ...mockContext, + extension: { + ...mockExtension, + outputPath: '/test/output/extension.js', + configuration: { + extensions: [ + {targeting: [{target: 'admin.intent.link', tools: './tools.json'}]}, + ], + }, + } as unknown as ExtensionInstance, + } + + vi.mocked(fs.fileExists).mockResolvedValue(false) + + const step: LifecycleStep = { + id: 'gen-manifest', + name: 'Generate Manifest', + type: 'include_assets', + config: { + generateManifest: true, + inclusions: [ + { + type: 'configKey', + key: 'extensions[].targeting[].tools', + anchor: 'extensions[].targeting[]', + groupBy: 'target', + }, + ], + }, + } + + // When + await executeIncludeAssetsStep(step, contextWithConfig) + + // Then — manifest is placed under /test/output, which is dirname of extension.js + expect(fs.writeFile).toHaveBeenCalledOnce() + const writeFileCall = vi.mocked(fs.writeFile).mock.calls[0]! + expect(writeFileCall[0]).toBe('/test/output/manifest.json') + }) + + test('still copies files AND writes manifest when generateManifest is true', async () => { + // Given + const contextWithConfig = { + ...mockContext, + extension: { + ...mockExtension, + configuration: { + extensions: [ + {targeting: [{target: 'admin.intent.link', tools: './tools.json'}]}, + ], + }, + } as unknown as ExtensionInstance, + } + + vi.mocked(fs.glob).mockResolvedValue([]) + + const step: LifecycleStep = { + id: 'gen-manifest', + name: 'Generate Manifest', + type: 'include_assets', + config: { + generateManifest: true, + inclusions: [ + { + type: 'configKey', + key: 'extensions[].targeting[].tools', + anchor: 'extensions[].targeting[]', + groupBy: 'target', + }, + ], + }, + } + + // When + await executeIncludeAssetsStep(step, contextWithConfig) + + // Then — file copying happened AND manifest was written + // joinPath normalises './tools.json' → 'tools.json', so the resolved source path has no leading './' + expect(fs.copyFile).toHaveBeenCalledWith('/test/extension/tools.json', '/test/output/tools.json') + expect(fs.writeFile).toHaveBeenCalledOnce() + const writeFileCall = vi.mocked(fs.writeFile).mock.calls[0]! + const manifestContent = JSON.parse(writeFileCall[1] as string) + expect(manifestContent).toEqual({ + 'admin.intent.link': {tools: 'tools.json'}, + }) + }) + + test('includes the full item when anchor equals key (relPath is empty string)', async () => { + // Given — anchor === key, so stripAnchorPrefix returns "" and buildRelativeEntry returns the whole item + const contextWithConfig = { + ...mockContext, + extension: { + ...mockExtension, + configuration: { + extensions: [ + { + targeting: [ + {target: 'admin.intent.link', tools: './tools.json', url: '/editor'}, + ], + }, + ], + }, + } as unknown as ExtensionInstance, + } + + vi.mocked(fs.fileExists).mockResolvedValue(false) + + const step: LifecycleStep = { + id: 'gen-manifest', + name: 'Generate Manifest', + type: 'include_assets', + config: { + generateManifest: true, + inclusions: [ + { + type: 'configKey', + // anchor === key → the whole targeting item becomes the manifest value + key: 'extensions[].targeting[]', + anchor: 'extensions[].targeting[]', + groupBy: 'target', + }, + ], + }, + } + + // When + await executeIncludeAssetsStep(step, contextWithConfig) + + // Then — manifest value is the full targeting object (including url) + expect(fs.writeFile).toHaveBeenCalledOnce() + const writeFileCall = vi.mocked(fs.writeFile).mock.calls[0]! + const manifestContent = JSON.parse(writeFileCall[1] as string) + expect(manifestContent).toEqual({ + 'admin.intent.link': { + target: 'admin.intent.link', + tools: './tools.json', + url: '/editor', + }, + }) + }) + }) }) diff --git a/packages/app/src/cli/services/build/steps/include-assets-step.ts b/packages/app/src/cli/services/build/steps/include-assets-step.ts index 0ae41d8b699..6c0a09217dc 100644 --- a/packages/app/src/cli/services/build/steps/include-assets-step.ts +++ b/packages/app/src/cli/services/build/steps/include-assets-step.ts @@ -1,3 +1,4 @@ +import {generateManifestFile} from './include-assets/generate-manifest.js' import {copyByPattern} from './include-assets/copy-by-pattern.js' import {copySourceEntry} from './include-assets/copy-source-entry.js' import {copyConfigKeyEntry} from './include-assets/copy-config-key-entry.js' @@ -35,12 +36,19 @@ const StaticEntrySchema = z.object({ * * Resolves a path (or array of paths) from the extension configuration and * copies the directory contents into the output. Silently skipped when the - * key is absent. + * key is absent. Respects `preserveStructure` and `destination` the same way + * as the static entry. + * + * `anchor` and `groupBy` are optional fields used for manifest generation. + * When both are present, this entry participates in `generateManifestFile`. */ const ConfigKeyEntrySchema = z.object({ type: z.literal('configKey'), key: z.string(), destination: z.string().optional(), + preserveStructure: z.boolean().default(false), + anchor: z.string().optional(), + groupBy: z.string().optional(), }) const InclusionEntrySchema = z.discriminatedUnion('type', [PatternEntrySchema, StaticEntrySchema, ConfigKeyEntrySchema]) @@ -49,11 +57,34 @@ const InclusionEntrySchema = z.discriminatedUnion('type', [PatternEntrySchema, S * Configuration schema for include_assets step. * * `inclusions` is a flat array of entries, each with a `type` discriminant - * (`'static'`, `'configKey'`, or `'pattern'`). All entries are processed in parallel. + * (`'static'`, `'configKey'`, or `'pattern'`). `configKey` entries run + * sequentially (to avoid filesystem race conditions on shared output paths), + * then `pattern` and `static` entries run in parallel. + * + * When `generateManifest` is `true`, a `manifest.json` file is written to the + * output directory after all inclusions complete. Only `configKey` entries + * that have both `anchor` and `groupBy` set participate in manifest generation. */ -const IncludeAssetsConfigSchema = z.object({ - inclusions: z.array(InclusionEntrySchema), -}) +const IncludeAssetsConfigSchema = z + .object({ + inclusions: z.array(InclusionEntrySchema), + generateManifest: z.boolean().default(false), + }) + .superRefine((data, ctx) => { + for (const [i, entry] of data.inclusions.entries()) { + if (entry.type === 'configKey') { + const hasAnchor = entry.anchor !== undefined + const hasGroupBy = entry.groupBy !== undefined + if (hasAnchor !== hasGroupBy) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + message: '`anchor` and `groupBy` must both be set or both be omitted', + path: ['inclusions', i], + }) + } + } + } + }) /** * Executes an include_assets build step. @@ -63,6 +94,7 @@ const IncludeAssetsConfigSchema = z.object({ * - `type: 'static'` — copy a file or directory into the output. * - `type: 'configKey'` — resolve a path from the extension's * config and copy into the output; silently skipped if absent. + * Runs sequentially to avoid filesystem race conditions. * - `type: 'pattern'` — glob-based file selection from a source directory * (defaults to extension root when `source` is omitted). */ @@ -76,51 +108,76 @@ export async function executeIncludeAssetsStep( // parent. When outputPath has no extension, it IS the output directory. const outputDir = extname(extension.outputPath) ? dirname(extension.outputPath) : extension.outputPath - const counts = await Promise.all( - config.inclusions.map(async (entry) => { - const warn = (msg: string) => options.stdout.write(msg) - const sanitizedDest = entry.destination === undefined ? undefined : sanitizeRelativePath(entry.destination, warn) + const aggregatedPathMap = new Map() - if (entry.type === 'pattern') { - const sourceDir = entry.baseDir ? joinPath(extension.directory, entry.baseDir) : extension.directory - const destinationDir = sanitizedDest ? joinPath(outputDir, sanitizedDest) : outputDir - return copyByPattern( - { - sourceDir, - outputDir: destinationDir, - patterns: entry.include, - ignore: entry.ignore ?? [], - }, - options, - ) - } + // configKey entries run sequentially: copyConfigKeyEntry uses findUniqueDestPath + // which checks filesystem state before writing. Running two configKey entries in + // parallel against the same output directory can cause both to see the same + // candidate path as free and silently overwrite each other. + let configKeyCount = 0 + for (const entry of config.inclusions) { + if (entry.type !== 'configKey') continue + const warn = (msg: string) => options.stdout.write(msg) + const rawDest = entry.destination ? sanitizeRelativePath(entry.destination, warn) : undefined + const sanitizedDest = rawDest === '' ? undefined : rawDest + // eslint-disable-next-line no-await-in-loop + const result = await copyConfigKeyEntry( + { + key: entry.key, + baseDir: extension.directory, + outputDir, + context, + destination: sanitizedDest, + }, + options, + ) + result.pathMap.forEach((val, key) => aggregatedPathMap.set(key, val)) + configKeyCount += result.filesCopied + } - if (entry.type === 'configKey') { - return copyConfigKeyEntry( - { - key: entry.key, - baseDir: extension.directory, - outputDir, - context, - destination: sanitizedDest, - }, - options, - ) - } + // pattern and static entries do not use findUniqueDestPath and do not + // contribute to the pathMap, so they are safe to run in parallel. + const otherCounts = await Promise.all( + config.inclusions + .filter((entry) => entry.type !== 'configKey') + .map(async (entry) => { + const warn = (msg: string) => options.stdout.write(msg) + const rawDest = entry.destination ? sanitizeRelativePath(entry.destination, warn) : undefined + const sanitizedDest = rawDest === '' ? undefined : rawDest - if (entry.type === 'static') { - return copySourceEntry( - { - source: entry.source, - destination: sanitizedDest, - baseDir: extension.directory, - outputDir, - }, - options, - ) - } - }), + if (entry.type === 'pattern') { + const sourceDir = entry.baseDir ? joinPath(extension.directory, entry.baseDir) : extension.directory + const destinationDir = sanitizedDest ? joinPath(outputDir, sanitizedDest) : outputDir + return copyByPattern( + { + sourceDir, + outputDir: destinationDir, + patterns: entry.include, + ignore: entry.ignore ?? [], + }, + options, + ) + } + + if (entry.type === 'static') { + return copySourceEntry( + { + source: entry.source, + destination: sanitizedDest, + baseDir: extension.directory, + outputDir, + }, + options, + ) + } + }), ) + if (config.generateManifest) { + const configKeyEntries = config.inclusions.filter((inclusion) => inclusion.type === 'configKey') + await generateManifestFile(configKeyEntries, context, outputDir, aggregatedPathMap) + } + + const counts = [configKeyCount, ...otherCounts] return {filesCopied: counts.reduce((sum, count) => sum + (count ?? 0), 0)} } diff --git a/packages/app/src/cli/services/build/steps/include-assets/copy-config-key-entry.ts b/packages/app/src/cli/services/build/steps/include-assets/copy-config-key-entry.ts index 946d0caade9..9b400a22dd0 100644 --- a/packages/app/src/cli/services/build/steps/include-assets/copy-config-key-entry.ts +++ b/packages/app/src/cli/services/build/steps/include-assets/copy-config-key-entry.ts @@ -1,4 +1,4 @@ -import {joinPath, basename} from '@shopify/cli-kit/node/path' +import {joinPath, basename, relativePath, extname} from '@shopify/cli-kit/node/path' import {glob, copyFile, copyDirectoryContents, fileExists, mkdir, isDirectory} from '@shopify/cli-kit/node/fs' import type {BuildContext} from '../../client-steps.js' @@ -9,6 +9,15 @@ import type {BuildContext} from '../../client-steps.js' * arrays are each used as source paths. Unresolved keys and missing paths are * skipped silently with a log message. When `destination` is given, the * resolved directory is placed under `outputDir/destination`. + * + * File sources are copied with `copyFile` using a unique destination name + * (via `findUniqueDestPath`) to prevent overwrites when multiple config values + * resolve to files with the same basename. Directory sources use + * `copyDirectoryContents`. + * + * Returns `{filesCopied, pathMap}` where `pathMap` maps each raw config path + * value to its output-relative location. File sources map to a single string. + * Directory sources map to a `string[]` of every output-relative file path. */ export async function copyConfigKeyEntry( config: { @@ -19,8 +28,8 @@ export async function copyConfigKeyEntry( destination?: string }, options: {stdout: NodeJS.WritableStream}, -): Promise { - const {key, baseDir, outputDir, context, destination} = config +): Promise<{filesCopied: number; pathMap: Map}> { + const {key, baseDir, outputDir, context, preserveStructure, destination} = config const value = getNestedValue(context.extension.configuration, key) let paths: string[] if (typeof value === 'string') { @@ -33,33 +42,79 @@ export async function copyConfigKeyEntry( if (paths.length === 0) { options.stdout.write(`No value for configKey '${key}', skipping\n`) - return 0 + return {filesCopied: 0, pathMap: new Map()} } const effectiveOutputDir = destination ? joinPath(outputDir, destination) : outputDir - const counts = await Promise.all( - paths.map(async (sourcePath) => { - const fullPath = joinPath(baseDir, sourcePath) - const exists = await fileExists(fullPath) - if (!exists) { - options.stdout.write(`Warning: path '${sourcePath}' does not exist, skipping\n`) - return 0 - } - if (!(await isDirectory(fullPath))) { - const destPath = joinPath(effectiveOutputDir, basename(fullPath)) - await mkdir(effectiveOutputDir) - await copyFile(fullPath, destPath) - options.stdout.write(`Included '${sourcePath}'\n`) - return 1 - } - await copyDirectoryContents(fullPath, effectiveOutputDir) - const copied = await glob(['**/*'], {cwd: effectiveOutputDir, absolute: false}) - options.stdout.write(`Included '${sourcePath}'\n`) - return copied.length - }), - ) - return counts.reduce((sum, count) => sum + count, 0) + // Deduplicate: the same source path shared across multiple targets + // should only be copied once; the pathMap entry is reused for all references. + const uniquePaths = [...new Set(paths)] + + // Process sequentially — findUniqueDestPath relies on filesystem state that + // would race if multiple copies ran in parallel against the same output dir. + const pathMap = new Map() + let filesCopied = 0 + + /* eslint-disable no-await-in-loop */ + for (const sourcePath of uniquePaths) { + const fullPath = joinPath(baseDir, sourcePath) + const exists = await fileExists(fullPath) + if (!exists) { + options.stdout.write(`Warning: path '${sourcePath}' does not exist, skipping\n`) + continue + } + + const sourceIsDir = await isDirectory(fullPath) + + const destDir = + sourceIsDir && preserveStructure ? joinPath(effectiveOutputDir, basename(fullPath)) : effectiveOutputDir + + if (sourceIsDir) { + await copyDirectoryContents(fullPath, destDir) + const copied = await glob(['**/*'], {cwd: destDir, absolute: false}) + const msg = preserveStructure + ? `Copied '${sourcePath}' to ${basename(fullPath)}\n` + : `Copied contents of '${sourcePath}' to output root\n` + options.stdout.write(msg) + const relFiles = copied.map((file) => relativePath(outputDir, joinPath(destDir, file))) + pathMap.set(sourcePath, relFiles) + filesCopied += copied.length + } else { + await mkdir(destDir) + const uniqueDestPath = await findUniqueDestPath(destDir, basename(fullPath)) + await copyFile(fullPath, uniqueDestPath) + const outputRelative = relativePath(outputDir, uniqueDestPath) + options.stdout.write(`Copied '${sourcePath}' to ${outputRelative}\n`) + pathMap.set(sourcePath, outputRelative) + filesCopied += 1 + } + } + /* eslint-enable no-await-in-loop */ + + return {filesCopied, pathMap} +} + +/** + * Returns a destination path for `filename` inside `dir` that does not already + * exist. If `dir/filename` is free, returns it as-is. Otherwise appends a + * counter before the extension: `name-1.ext`, `name-2.ext`, … + */ +async function findUniqueDestPath(dir: string, filename: string): Promise { + const candidate = joinPath(dir, filename) + if (!(await fileExists(candidate))) return candidate + + const ext = extname(filename) + const base = ext ? filename.slice(0, -ext.length) : filename + let counter = 1 + const maxAttempts = 1000 + while (counter <= maxAttempts) { + const next = joinPath(dir, `${base}-${counter}${ext}`) + // eslint-disable-next-line no-await-in-loop + if (!(await fileExists(next))) return next + counter++ + } + throw new Error(`Unable to find unique destination path for '${filename}' in '${dir}' after ${maxAttempts} attempts`) } /** @@ -74,7 +129,7 @@ export async function copyConfigKeyEntry( * "targeting.tools" → [name:"targeting",...], [name:"tools",...] * "extensions[].targeting[].schema" → [name:"extensions", flatten:true], ... */ -function tokenizePath(path: string): {name: string; flatten: boolean}[] { +export function tokenizePath(path: string): {name: string; flatten: boolean}[] { return path.split('.').map((part) => { const flatten = part.endsWith('[]') return {name: flatten ? part.slice(0, -2) : part, flatten} @@ -92,7 +147,7 @@ function tokenizePath(path: string): {name: string; flatten: boolean}[] { * of nesting. Returns `undefined` if the value at that point is not an array * — the `[]` suffix is a contract that an array is expected there. */ -function getNestedValue(obj: {[key: string]: unknown}, path: string): unknown { +export function getNestedValue(obj: {[key: string]: unknown}, path: string): unknown { let current: unknown = obj for (const {name, flatten} of tokenizePath(path)) { diff --git a/packages/app/src/cli/services/build/steps/include-assets/generate-manifest.ts b/packages/app/src/cli/services/build/steps/include-assets/generate-manifest.ts new file mode 100644 index 00000000000..754cfaa42b9 --- /dev/null +++ b/packages/app/src/cli/services/build/steps/include-assets/generate-manifest.ts @@ -0,0 +1,222 @@ +import {getNestedValue, tokenizePath} from './copy-config-key-entry.js' +import {joinPath} from '@shopify/cli-kit/node/path' +import {fileExists, mkdir, writeFile} from '@shopify/cli-kit/node/fs' +import type {BuildContext} from '../../client-steps.js' + +type ConfigKeyManifestEntry = { + anchor?: string | undefined + groupBy?: string | undefined + key: string +} + +/** + * Generates a `manifest.json` file in `outputDir` from `configKey` inclusions. + * + * Algorithm: + * 1. Partition entries into anchored (both `anchor` and `groupBy` set) and + * root-level (neither set). + * 2. Return early when there is nothing to write. + * 3. Build root-level entries. + * 4. Build grouped entries (anchor/groupBy logic) with path strings resolved + * via `resolveManifestPaths` using the copy-tracked `pathMap`. + * 5. Write `outputDir/manifest.json`; throw if the file already exists. + * + * @param pathMap - Map from raw config path values to their output-relative + * paths, as recorded during the copy phase by `copyConfigKeyEntry`. + */ +export async function generateManifestFile( + configKeyEntries: ConfigKeyManifestEntry[], + context: BuildContext, + outputDir: string, + pathMap: Map, +): Promise { + const {extension, options} = context + + type AnchoredEntry = ConfigKeyManifestEntry & {anchor: string; groupBy: string} + + const anchoredIncs: AnchoredEntry[] = [] + const rootIncs: ConfigKeyManifestEntry[] = [] + + for (const entry of configKeyEntries) { + if (typeof entry.anchor === 'string' && typeof entry.groupBy === 'string') { + anchoredIncs.push(entry as AnchoredEntry) + } else { + rootIncs.push(entry) + } + } + + if (anchoredIncs.length === 0 && rootIncs.length === 0) return + + const manifest: {[key: string]: unknown} = {} + + // Root-level entries + for (const inc of rootIncs) { + const key = lastKeySegment(inc.key) + const rawValue = getNestedValue(extension.configuration, inc.key) + if (rawValue === null || rawValue === undefined) continue + manifest[key] = resolveManifestPaths(rawValue, pathMap) + } + + // Anchored grouped entries — group by (anchor, groupBy) pair + const groups = new Map() + for (const inclusion of anchoredIncs) { + const groupKey = `${inclusion.anchor}||${inclusion.groupBy}` + const existing = groups.get(groupKey) + if (existing) { + existing.push(inclusion) + } else { + groups.set(groupKey, [inclusion]) + } + } + + for (const inclusions of groups.values()) { + const {anchor, groupBy} = inclusions[0]! + const anchorValue = getNestedValue(extension.configuration, anchor) + if (!Array.isArray(anchorValue)) continue + + for (const item of anchorValue) { + if (item === null || typeof item !== 'object' || Array.isArray(item)) continue + const typedItem = item as {[key: string]: unknown} + + const manifestKey = typedItem[groupBy] + if (typeof manifestKey !== 'string') continue + + const partials = inclusions.map((inclusion) => { + const relPath = stripAnchorPrefix(inclusion.key, anchor) + const partial = buildRelativeEntry(typedItem, relPath) + return resolveManifestPaths(partial, pathMap) as {[key: string]: unknown} + }) + + const hasUnresolved = partials.some(containsUnresolvedPath) + if (hasUnresolved) { + options.stdout.write( + `Warning: manifest entry '${manifestKey}' contains unresolved paths — source files may be missing\n`, + ) + } + + if (Object.prototype.hasOwnProperty.call(manifest, manifestKey)) { + options.stdout.write(`Warning: duplicate manifest key '${manifestKey}' — later entry overwrites earlier one\n`) + } + manifest[manifestKey] = shallowMerge(partials) + } + } + + if (Object.keys(manifest).length === 0) { + options.stdout.write('Warning: no manifest entries produced — skipping manifest.json\n') + return + } + + const manifestPath = joinPath(outputDir, 'manifest.json') + if (await fileExists(manifestPath)) { + throw new Error( + `Can't write manifest.json: a file already exists at '${manifestPath}'. ` + + `Remove or rename the conflicting inclusion to avoid overwriting the generated manifest.`, + ) + } + await mkdir(outputDir) + await writeFile(manifestPath, JSON.stringify(manifest, null, 2)) + options.stdout.write(`Generated manifest.json in ${outputDir}\n`) +} + +/** + * Strips the anchor prefix (plus trailing dot separator) from a config key path. + * + * Examples: + * anchor = "extensions[].targeting[]", key = "extensions[].targeting[].tools" + * → "tools" + * anchor === key → "" (include the whole item) + * key does not start with anchor → key returned as-is + */ +function stripAnchorPrefix(key: string, anchor: string): string { + if (anchor === key) return '' + const prefix = `${anchor}.` + if (key.startsWith(prefix)) return key.slice(prefix.length) + return key +} + +/** + * Builds a partial manifest object from an item and a relative path string. + * + * - `""` → returns the item itself + * - `"tools"` → `{tools: item.tools}` + * - `"intents[].schema"` → `{intents: item.intents.map(el => buildRelativeEntry(el, "schema"))}` + */ +function buildRelativeEntry(item: {[key: string]: unknown}, relPath: string): {[key: string]: unknown} { + if (relPath === '') return item + + const tokens = tokenizePath(relPath) + const [head, ...rest] = tokens + if (!head) return item + const restPath = rest.map((t) => `${t.name}${t.flatten ? '[]' : ''}`).join('.') + + const value = item[head.name] + + if (head.flatten) { + if (!Array.isArray(value)) return {[head.name]: value} + const mapped = (value as {[key: string]: unknown}[]).map((el) => (restPath ? buildRelativeEntry(el, restPath) : el)) + return {[head.name]: mapped} + } + + if (restPath && value !== null && value !== undefined && typeof value === 'object' && !Array.isArray(value)) { + return {[head.name]: buildRelativeEntry(value as {[key: string]: unknown}, restPath)} + } + + return {[head.name]: value} +} + +/** + * Merges multiple partial objects into one (shallow / top-level keys). + */ +function shallowMerge(objects: {[key: string]: unknown}[]): {[key: string]: unknown} { + return Object.assign({}, ...objects) +} + +/** + * Resolves raw config path values to their output-relative paths using the + * copy-tracked path map. Strings not in the map (and not path-like) are left + * unchanged. Walks objects and arrays recursively. + * + * When a pathMap value is a `string[]` (directory source), the string is + * replaced with the array — producing a file-list manifest entry. + */ +function resolveManifestPaths(value: unknown, pathMap: Map): unknown { + if (typeof value === 'string') { + const looksLikePath = value.startsWith('.') || value.includes('/') || value.includes('\\') || pathMap.has(value) + return (looksLikePath ? pathMap.get(value) : undefined) ?? value + } + if (Array.isArray(value)) return value.map((el) => resolveManifestPaths(el, pathMap)) + if (value !== null && typeof value === 'object') { + const result: {[key: string]: unknown} = {} + for (const [key, val] of Object.entries(value as {[key: string]: unknown})) { + result[key] = resolveManifestPaths(val, pathMap) + } + return result + } + return value +} + +/** + * Returns the last dot-separated segment of a key path, stripping any `[]` suffix. + * + * Examples: + * `"tools"` → `"tools"` + * `"targeting.tools"` → `"tools"` + * `"extensions[].targeting[].tools"` → `"tools"` + */ +function lastKeySegment(key: string): string { + const last = key.split('.').at(-1) ?? key + return last.endsWith('[]') ? last.slice(0, -2) : last +} + +/** + * Returns `true` if `value` contains any string — at any depth — that looks + * like an unresolved config path (starts with `./` or `../`). + */ +function containsUnresolvedPath(value: unknown): boolean { + if (typeof value === 'string') return value.startsWith('./') || value.startsWith('../') + if (Array.isArray(value)) return value.some(containsUnresolvedPath) + if (value !== null && typeof value === 'object') { + return Object.values(value as {[key: string]: unknown}).some(containsUnresolvedPath) + } + return false +} From f56e2e258ad15aef4fef9584b0c4031dac225766 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Wed, 1 Apr 2026 11:25:58 +0200 Subject: [PATCH 2/5] Fix lint and type errors in include_assets step - Rename short identifiers (k, v, p) to satisfy id-length rule - Add eslint-disable for intentional no-await-in-loop in sequential copy loop - Add null guard for head in buildRelativeEntry to fix TS18048 - Run prettier fixes Co-authored-by: Claude Code --- .../build/steps/include-assets-step.test.ts | 337 ++++++++++++++---- .../build/steps/include-assets-step.ts | 3 +- 2 files changed, 278 insertions(+), 62 deletions(-) diff --git a/packages/app/src/cli/services/build/steps/include-assets-step.test.ts b/packages/app/src/cli/services/build/steps/include-assets-step.test.ts index 07d2e9cf731..a3101a0498f 100644 --- a/packages/app/src/cli/services/build/steps/include-assets-step.test.ts +++ b/packages/app/src/cli/services/build/steps/include-assets-step.test.ts @@ -171,12 +171,13 @@ describe('executeIncludeAssetsStep', () => { expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('Included README.md')) }) - test('copies a directory to explicit destination path', async () => { + test('copies a directory to explicit destination path and returns actual file count', async () => { // Given vi.mocked(fs.fileExists).mockResolvedValue(true) vi.mocked(fs.isDirectory).mockResolvedValue(true) vi.mocked(fs.copyDirectoryContents).mockResolvedValue() - vi.mocked(fs.glob).mockResolvedValue(['a.js', 'b.js']) + vi.mocked(fs.glob).mockResolvedValue(['a.js', 'b.js', 'c.js']) + vi.mocked(fs.mkdir).mockResolvedValue() const step: LifecycleStep = { id: 'copy-dist', @@ -190,10 +191,11 @@ describe('executeIncludeAssetsStep', () => { // When const result = await executeIncludeAssetsStep(step, mockContext) - // Then + // Then — uses copyDirectoryContents (not copyFile) and counts actual files via glob expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/test/extension/dist', '/test/output/assets/dist') - expect(result.filesCopied).toBe(2) - expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('Included dist')) + expect(fs.copyFile).not.toHaveBeenCalled() + expect(result.filesCopied).toBe(3) + expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('Copied dist to assets/dist')) }) }) @@ -289,6 +291,75 @@ describe('executeIncludeAssetsStep', () => { ) }) + test('renames output file to avoid collision when candidate path already exists', async () => { + // Given — tools.json already exists in the output dir; findUniqueDestPath must try tools-1.json + const contextWithConfig = { + ...mockContext, + extension: { + ...mockExtension, + configuration: {tools: './tools.json'}, + } as unknown as ExtensionInstance, + } + + vi.mocked(fs.fileExists).mockImplementation(async (path) => { + const pathStr = String(path) + // Source file exists; first candidate output path is taken; suffixed path is free + return pathStr === '/test/extension/tools.json' || pathStr === '/test/output/tools.json' + }) + vi.mocked(fs.isDirectory).mockResolvedValue(false) + vi.mocked(fs.copyFile).mockResolvedValue() + vi.mocked(fs.mkdir).mockResolvedValue() + + const step: LifecycleStep = { + id: 'copy-tools', + name: 'Copy Tools', + type: 'include_assets', + config: { + inclusions: [{type: 'configKey', key: 'tools'}], + }, + } + + // When + const result = await executeIncludeAssetsStep(step, contextWithConfig) + + // Then — copied to tools-1.json, not tools.json + expect(fs.copyFile).toHaveBeenCalledWith('/test/extension/tools.json', '/test/output/tools-1.json') + expect(result.filesCopied).toBe(1) + }) + + test('throws after exhausting 1000 rename attempts', async () => { + // Given — every candidate path (tools.json, tools-1.json … tools-1000.json) is taken + const contextWithConfig = { + ...mockContext, + extension: { + ...mockExtension, + configuration: {tools: './tools.json'}, + } as unknown as ExtensionInstance, + } + + vi.mocked(fs.fileExists).mockImplementation(async (path) => { + const pathStr = String(path) + // Source file exists; all 1001 output candidates are occupied + return pathStr.startsWith('/test/extension/') || pathStr.startsWith('/test/output/tools') + }) + vi.mocked(fs.isDirectory).mockResolvedValue(false) + vi.mocked(fs.mkdir).mockResolvedValue() + + const step: LifecycleStep = { + id: 'copy-tools', + name: 'Copy Tools', + type: 'include_assets', + config: { + inclusions: [{type: 'configKey', key: 'tools'}], + }, + } + + // When / Then + await expect(executeIncludeAssetsStep(step, contextWithConfig)).rejects.toThrow( + "Unable to find unique destination path for 'tools.json' in '/test/output' after 1000 attempts", + ) + }) + test('resolves array config value and copies each path', async () => { // Given — static_root is an array const contextWithArrayConfig = { @@ -336,10 +407,14 @@ describe('executeIncludeAssetsStep', () => { } as unknown as ExtensionInstance, } - vi.mocked(fs.fileExists).mockResolvedValue(true) - vi.mocked(fs.isDirectory).mockResolvedValue(true) + vi.mocked(fs.fileExists).mockImplementation( + async (path) => typeof path === 'string' && path.startsWith('/test/extension'), + ) + vi.mocked(fs.isDirectory).mockResolvedValue(false) vi.mocked(fs.copyDirectoryContents).mockResolvedValue() vi.mocked(fs.glob).mockResolvedValue(['file.js']) + vi.mocked(fs.copyFile).mockResolvedValue() + vi.mocked(fs.mkdir).mockResolvedValue() const step: LifecycleStep = { id: 'copy-tools', @@ -397,7 +472,8 @@ describe('executeIncludeAssetsStep', () => { } vi.mocked(fs.fileExists).mockResolvedValue(true) - vi.mocked(fs.isDirectory).mockImplementation((path) => Promise.resolve(!String(path).endsWith('.png'))) + // Directories have no file extension; files do + vi.mocked(fs.isDirectory).mockImplementation(async (path) => !/\.\w+$/.test(String(path))) vi.mocked(fs.copyDirectoryContents).mockResolvedValue() vi.mocked(fs.copyFile).mockResolvedValue() vi.mocked(fs.mkdir).mockResolvedValue() @@ -418,7 +494,7 @@ describe('executeIncludeAssetsStep', () => { // When const result = await executeIncludeAssetsStep(step, contextWithConfig) - // Then + // Then — directory configKey uses copyDirectoryContents; file static uses copyFile expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/test/extension/public', '/test/output') expect(fs.copyFile).toHaveBeenCalledWith('/test/extension/src/icon.png', '/test/output/assets/icon.png') expect(result.filesCopied).toBe(2) @@ -550,14 +626,16 @@ describe('executeIncludeAssetsStep', () => { } vi.mocked(fs.fileExists).mockResolvedValue(true) - vi.mocked(fs.isDirectory).mockImplementation((path) => Promise.resolve(!String(path).endsWith('.json'))) + // Directories have no file extension; files do + vi.mocked(fs.isDirectory).mockImplementation(async (path) => !/\.\w+$/.test(String(path))) vi.mocked(fs.copyDirectoryContents).mockResolvedValue() vi.mocked(fs.copyFile).mockResolvedValue() vi.mocked(fs.mkdir).mockResolvedValue() - // glob: first call for pattern entry, second for configKey dir listing + // configKey entries run sequentially first, then pattern/static in parallel. + // glob: first call for configKey dir listing, second for pattern source files. vi.mocked(fs.glob) - .mockResolvedValueOnce(['/test/extension/assets/logo.png', '/test/extension/assets/icon.svg']) .mockResolvedValueOnce(['index.html', 'style.css']) + .mockResolvedValueOnce(['/test/extension/assets/logo.png', '/test/extension/assets/icon.svg']) const step: LifecycleStep = { id: 'include-all', @@ -576,7 +654,7 @@ describe('executeIncludeAssetsStep', () => { const result = await executeIncludeAssetsStep(step, contextWithConfig) // Then - // 5 = 2 pattern + 2 configKey dir contents + 1 explicit file + // 5 = 2 pattern + 2 configKey dir contents + 1 explicit file (manifest.json is a file → copyFile → 1) expect(result.filesCopied).toBe(5) expect(fs.copyFile).toHaveBeenCalledWith('/test/extension/src/manifest.json', '/test/output/manifest.json') expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/test/extension/theme', '/test/output') @@ -590,8 +668,8 @@ describe('executeIncludeAssetsStep', () => { // Source files exist; destination paths don't yet (so findUniqueDestPath // resolves on the first candidate without looping). Individual tests can // override for specific scenarios. - vi.mocked(fs.fileExists).mockImplementation(async (p) => - typeof p === 'string' && p.startsWith('/test/extension'), + vi.mocked(fs.fileExists).mockImplementation( + async (path) => typeof path === 'string' && path.startsWith('/test/extension'), ) vi.mocked(fs.copyFile).mockResolvedValue() vi.mocked(fs.copyDirectoryContents).mockResolvedValue() @@ -607,17 +685,13 @@ describe('executeIncludeAssetsStep', () => { configuration: { extensions: [ { - targeting: [ - {target: 'admin.app.intent.link', tools: './tools.json', url: '/editor'}, - ], + targeting: [{target: 'admin.app.intent.link', tools: './tools.json', url: '/editor'}], }, ], }, } as unknown as ExtensionInstance, } - - const step: LifecycleStep = { id: 'gen-manifest', name: 'Generate Manifest', @@ -674,8 +748,6 @@ describe('executeIncludeAssetsStep', () => { } as unknown as ExtensionInstance, } - - const step: LifecycleStep = { id: 'gen-manifest', name: 'Generate Manifest', @@ -740,8 +812,6 @@ describe('executeIncludeAssetsStep', () => { } as unknown as ExtensionInstance, } - - const step: LifecycleStep = { id: 'gen-manifest', name: 'Generate Manifest', @@ -838,9 +908,7 @@ describe('executeIncludeAssetsStep', () => { type: 'include_assets', config: { generateManifest: true, - inclusions: [ - {type: 'pattern', baseDir: 'public', include: ['**/*']}, - ], + inclusions: [{type: 'pattern', baseDir: 'public', include: ['**/*']}], }, } @@ -861,8 +929,6 @@ describe('executeIncludeAssetsStep', () => { } as unknown as ExtensionInstance, } - - const step: LifecycleStep = { id: 'gen-manifest', name: 'Generate Manifest', @@ -888,17 +954,82 @@ describe('executeIncludeAssetsStep', () => { }) }) - test('logs a warning and treats inclusion as root-level when only anchor is set (no groupBy)', async () => { - // Given — inclusion has anchor but no groupBy + test('maps a directory configKey to a file list in the manifest', async () => { + // Directory sources produce a string[] of output-relative file paths rather + // than an opaque directory marker like "." or "". const contextWithConfig = { ...mockContext, extension: { ...mockExtension, - configuration: {targeting: {tools: './tools.json'}}, + configuration: {admin: {static_root: 'dist'}}, } as unknown as ExtensionInstance, } - vi.mocked(fs.fileExists).mockResolvedValue(false) + vi.mocked(fs.fileExists).mockImplementation(async (pathArg) => { + // The source 'dist' directory must exist so the copy runs; manifest.json must not + return String(pathArg) === '/test/extension/dist' + }) + vi.mocked(fs.isDirectory).mockResolvedValue(true) + vi.mocked(fs.copyDirectoryContents).mockResolvedValue() + vi.mocked(fs.glob).mockResolvedValue(['index.html', 'style.css']) + + const step: LifecycleStep = { + id: 'copy-static', + name: 'Copy Static', + type: 'include_assets', + config: { + generateManifest: true, + // no destination, no preserveStructure → contents merged into output root + inclusions: [{type: 'configKey', key: 'admin.static_root'}], + }, + } + + // When + await executeIncludeAssetsStep(step, contextWithConfig) + + // Then — directory produces a file list, not an opaque directory marker + expect(fs.writeFile).toHaveBeenCalledOnce() + const manifestContent = JSON.parse(vi.mocked(fs.writeFile).mock.calls[0]![1] as string) + expect(manifestContent).toEqual({static_root: ['index.html', 'style.css']}) + }) + + test('throws a validation error when only anchor is set without groupBy', async () => { + // Given — inclusion has anchor but no groupBy — schema now rejects this at parse time + const step: LifecycleStep = { + id: 'gen-manifest', + name: 'Generate Manifest', + type: 'include_assets', + config: { + generateManifest: true, + inclusions: [{type: 'configKey', key: 'targeting.tools', anchor: 'targeting'}], + }, + } + + // When / Then — schema refinement rejects anchor without groupBy + await expect(executeIncludeAssetsStep(step, mockContext)).rejects.toThrow( + '`anchor` and `groupBy` must both be set or both be omitted', + ) + }) + + test('throws when manifest.json already exists in the output directory', async () => { + // Given — a prior inclusion already copied a manifest.json to the output dir + const contextWithConfig = { + ...mockContext, + extension: { + ...mockExtension, + configuration: { + extensions: [{targeting: [{target: 'admin.intent.link', tools: './tools.json'}]}], + }, + } as unknown as ExtensionInstance, + } + + // Source files exist; output manifest.json already exists (simulating conflict); + // candidate output paths for tools.json are free so copyConfigKeyEntry succeeds. + vi.mocked(fs.fileExists).mockImplementation(async (path) => { + const pathStr = String(path) + return pathStr === '/test/output/manifest.json' || pathStr.startsWith('/test/extension/') + }) + vi.mocked(fs.glob).mockResolvedValue([]) const step: LifecycleStep = { id: 'gen-manifest', @@ -907,20 +1038,20 @@ describe('executeIncludeAssetsStep', () => { config: { generateManifest: true, inclusions: [ - {type: 'configKey', key: 'targeting.tools', anchor: 'targeting'}, + { + type: 'configKey', + key: 'extensions[].targeting[].tools', + anchor: 'extensions[].targeting[]', + groupBy: 'target', + }, ], }, } - // When - await executeIncludeAssetsStep(step, contextWithConfig) - - // Then — warning logged, inclusion treated as root entry - expect(mockStdout.write).toHaveBeenCalledWith( - expect.stringContaining('anchor without groupBy (or vice versa)'), + // When / Then — throws rather than silently overwriting + await expect(executeIncludeAssetsStep(step, contextWithConfig)).rejects.toThrow( + `Can't write manifest.json: a file already exists at '/test/output/manifest.json'`, ) - const manifestContent = JSON.parse(vi.mocked(fs.writeFile).mock.calls[0]![1] as string) - expect(manifestContent).toHaveProperty('tools') }) test('writes an empty manifest when anchor resolves to a non-array value', async () => { @@ -936,8 +1067,6 @@ describe('executeIncludeAssetsStep', () => { } as unknown as ExtensionInstance, } - - const step: LifecycleStep = { id: 'gen-manifest', name: 'Generate Manifest', @@ -958,11 +1087,9 @@ describe('executeIncludeAssetsStep', () => { // When await executeIncludeAssetsStep(step, contextWithConfig) - // Then — the anchor group was skipped; manifest.json is written but contains no entries - expect(fs.writeFile).toHaveBeenCalledOnce() - const writeFileCall = vi.mocked(fs.writeFile).mock.calls[0]! - const manifestContent = JSON.parse(writeFileCall[1] as string) - expect(manifestContent).toEqual({}) + // Then — no entries produced; manifest.json is NOT written, warning is logged + expect(fs.writeFile).not.toHaveBeenCalled() + expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('no manifest entries produced')) }) test('skips items whose groupBy field is not a string', async () => { @@ -984,8 +1111,6 @@ describe('executeIncludeAssetsStep', () => { } as unknown as ExtensionInstance, } - - const step: LifecycleStep = { id: 'gen-manifest', name: 'Generate Manifest', @@ -1024,9 +1149,7 @@ describe('executeIncludeAssetsStep', () => { ...mockExtension, outputPath: '/test/output/extension.js', configuration: { - extensions: [ - {targeting: [{target: 'admin.intent.link', tools: './tools.json'}]}, - ], + extensions: [{targeting: [{target: 'admin.intent.link', tools: './tools.json'}]}], }, } as unknown as ExtensionInstance, } @@ -1066,9 +1189,7 @@ describe('executeIncludeAssetsStep', () => { extension: { ...mockExtension, configuration: { - extensions: [ - {targeting: [{target: 'admin.intent.link', tools: './tools.json'}]}, - ], + extensions: [{targeting: [{target: 'admin.intent.link', tools: './tools.json'}]}], }, } as unknown as ExtensionInstance, } @@ -1106,6 +1227,49 @@ describe('executeIncludeAssetsStep', () => { }) }) + test('resolves bare filename in manifest even without ./ prefix', async () => { + // Given — config value is a bare filename with no ./ prefix; pathMap.has() must catch it + const contextWithConfig = { + ...mockContext, + extension: { + ...mockExtension, + configuration: { + extensions: [{targeting: [{target: 'admin.intent.link', tools: 'tools.json'}]}], + }, + } as unknown as ExtensionInstance, + } + + vi.mocked(fs.glob).mockResolvedValue([]) + + const step: LifecycleStep = { + id: 'gen-manifest', + name: 'Generate Manifest', + type: 'include_assets', + config: { + generateManifest: true, + inclusions: [ + { + type: 'configKey', + key: 'extensions[].targeting[].tools', + anchor: 'extensions[].targeting[]', + groupBy: 'target', + }, + ], + }, + } + + // When + await executeIncludeAssetsStep(step, contextWithConfig) + + // Then — 'tools.json' (no ./ prefix) must be resolved to its output-relative path in the manifest + expect(fs.copyFile).toHaveBeenCalledWith('/test/extension/tools.json', '/test/output/tools.json') + const writeFileCall = vi.mocked(fs.writeFile).mock.calls[0]! + const manifestContent = JSON.parse(writeFileCall[1] as string) + expect(manifestContent).toEqual({ + 'admin.intent.link': {tools: 'tools.json'}, + }) + }) + test('includes the full item when anchor equals key (relPath is empty string)', async () => { // Given — anchor === key, so stripAnchorPrefix returns "" and buildRelativeEntry returns the whole item const contextWithConfig = { @@ -1115,9 +1279,7 @@ describe('executeIncludeAssetsStep', () => { configuration: { extensions: [ { - targeting: [ - {target: 'admin.intent.link', tools: './tools.json', url: '/editor'}, - ], + targeting: [{target: 'admin.intent.link', tools: './tools.json', url: '/editor'}], }, ], }, @@ -1147,7 +1309,9 @@ describe('executeIncludeAssetsStep', () => { // When await executeIncludeAssetsStep(step, contextWithConfig) - // Then — manifest value is the full targeting object (including url) + // Then — manifest value is the full targeting object (including url). + // tools: './tools.json' was never copied (configKey resolved to an object, not a string), + // so the path is left as-is and a warning is logged. expect(fs.writeFile).toHaveBeenCalledOnce() const writeFileCall = vi.mocked(fs.writeFile).mock.calls[0]! const manifestContent = JSON.parse(writeFileCall[1] as string) @@ -1158,6 +1322,57 @@ describe('executeIncludeAssetsStep', () => { url: '/editor', }, }) + expect(mockStdout.write).toHaveBeenCalledWith( + expect.stringContaining("manifest entry 'admin.intent.link' contains unresolved paths"), + ) + }) + + test('warns when a manifest entry contains an unresolved path because the source file was missing', async () => { + // Given — tools.json is referenced in config but does not exist on disk, + // so copyConfigKeyEntry skips it and it never enters pathMap. + const contextWithConfig = { + ...mockContext, + extension: { + ...mockExtension, + configuration: { + extensions: [{targeting: [{target: 'admin.intent.link', tools: './tools.json'}]}], + }, + } as unknown as ExtensionInstance, + } + + // Source file does not exist; output paths are free + vi.mocked(fs.fileExists).mockResolvedValue(false) + vi.mocked(fs.glob).mockResolvedValue([]) + + const step: LifecycleStep = { + id: 'gen-manifest', + name: 'Generate Manifest', + type: 'include_assets', + config: { + generateManifest: true, + inclusions: [ + { + type: 'configKey', + key: 'extensions[].targeting[].tools', + anchor: 'extensions[].targeting[]', + groupBy: 'target', + }, + ], + }, + } + + // When + await executeIncludeAssetsStep(step, contextWithConfig) + + // Then — raw './tools.json' appears in manifest (not copied → not resolved), + // and a diagnostic warning is logged so the user knows a file is missing. + const writeFileCall = vi.mocked(fs.writeFile).mock.calls[0]! + const manifestContent = JSON.parse(writeFileCall[1] as string) + expect(manifestContent).toEqual({'admin.intent.link': {tools: './tools.json'}}) + expect(mockStdout.write).toHaveBeenCalledWith( + expect.stringContaining("manifest entry 'admin.intent.link' contains unresolved paths"), + ) + expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining("path './tools.json' does not exist")) }) }) }) diff --git a/packages/app/src/cli/services/build/steps/include-assets-step.ts b/packages/app/src/cli/services/build/steps/include-assets-step.ts index 6c0a09217dc..4f6a086c9e3 100644 --- a/packages/app/src/cli/services/build/steps/include-assets-step.ts +++ b/packages/app/src/cli/services/build/steps/include-assets-step.ts @@ -173,11 +173,12 @@ export async function executeIncludeAssetsStep( }), ) + const counts = [configKeyCount, ...otherCounts] + if (config.generateManifest) { const configKeyEntries = config.inclusions.filter((inclusion) => inclusion.type === 'configKey') await generateManifestFile(configKeyEntries, context, outputDir, aggregatedPathMap) } - const counts = [configKeyCount, ...otherCounts] return {filesCopied: counts.reduce((sum, count) => sum + (count ?? 0), 0)} } From 922a528e8b6da29a67f950774371960e5db1e49d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Wed, 1 Apr 2026 11:25:59 +0200 Subject: [PATCH 3/5] Fix test assertions and lint errors in manifest branch - Update copy-config-key-entry.test.ts: use result.filesCopied instead of result (function now returns {filesCopied, pathMap}); fix fileExists mock in file copy tests so findUniqueDestPath resolves on first attempt - include-assets-step.ts: rename (e) => entry to satisfy id-length rule - generate-manifest.ts: use interface instead of type alias Co-authored-by: Claude Sonnet 4.6 Co-authored-by: Claude Code --- .../build/steps/include-assets-step.ts | 2 +- .../copy-config-key-entry.test.ts | 126 +++++++++++------- .../steps/include-assets/generate-manifest.ts | 2 +- 3 files changed, 79 insertions(+), 51 deletions(-) diff --git a/packages/app/src/cli/services/build/steps/include-assets-step.ts b/packages/app/src/cli/services/build/steps/include-assets-step.ts index 4f6a086c9e3..75c0f98c397 100644 --- a/packages/app/src/cli/services/build/steps/include-assets-step.ts +++ b/packages/app/src/cli/services/build/steps/include-assets-step.ts @@ -176,7 +176,7 @@ export async function executeIncludeAssetsStep( const counts = [configKeyCount, ...otherCounts] if (config.generateManifest) { - const configKeyEntries = config.inclusions.filter((inclusion) => inclusion.type === 'configKey') + const configKeyEntries = config.inclusions.filter((entry) => entry.type === 'configKey') await generateManifestFile(configKeyEntries, context, outputDir, aggregatedPathMap) } diff --git a/packages/app/src/cli/services/build/steps/include-assets/copy-config-key-entry.test.ts b/packages/app/src/cli/services/build/steps/include-assets/copy-config-key-entry.test.ts index 432fdadb0d2..b19afe0b78d 100644 --- a/packages/app/src/cli/services/build/steps/include-assets/copy-config-key-entry.test.ts +++ b/packages/app/src/cli/services/build/steps/include-assets/copy-config-key-entry.test.ts @@ -26,39 +26,48 @@ describe('copyConfigKeyEntry', () => { const outDir = joinPath(tmpDir, 'out') await mkdir(outDir) - const context = makeContext({static_root: 'public'}) - const result = await copyConfigKeyEntry( - {key: 'static_root', baseDir: tmpDir, outputDir: outDir, context}, - {stdout: mockStdout}, - ) + // Then + expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/ext/public', '/out') + expect(result.filesCopied).toBe(2) + expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('Copied contents of')) + }) - expect(result).toBe(2) - await expect(fileExists(joinPath(outDir, 'index.html'))).resolves.toBe(true) - await expect(fileExists(joinPath(outDir, 'logo.png'))).resolves.toBe(true) - expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining("Included 'public'")) - }) + test('places directory under its own name when preserveStructure is true', async () => { + // Given + const context = makeContext({theme_root: 'theme'}) + vi.mocked(fs.fileExists).mockResolvedValue(true) + vi.mocked(fs.isDirectory).mockResolvedValue(true) + vi.mocked(fs.copyDirectoryContents).mockResolvedValue() + vi.mocked(fs.glob).mockResolvedValue(['style.css', 'layout.liquid']) + + // When + const result = await copyConfigKeyEntry( + {key: 'theme_root', baseDir: '/ext', outputDir: '/out', context, preserveStructure: true}, + {stdout: mockStdout}, + ) + + // Then + expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/ext/theme', '/out/theme') + expect(result.filesCopied).toBe(2) + expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining("Copied 'theme' to theme")) }) test('copies a file source to outputDir/basename', async () => { - await inTemporaryDirectory(async (tmpDir) => { - const srcDir = joinPath(tmpDir, 'src') - await mkdir(srcDir) - await writeFile(joinPath(srcDir, 'schema.json'), '{}') + // Given + const context = makeContext({schema_path: 'src/schema.json'}) + // Source file exists; output path is free so findUniqueDestPath resolves on first attempt + vi.mocked(fs.fileExists).mockImplementation(async (p) => String(p) === '/ext/src/schema.json') + vi.mocked(fs.isDirectory).mockResolvedValue(false) + vi.mocked(fs.mkdir).mockResolvedValue() + vi.mocked(fs.copyFile).mockResolvedValue() const outDir = joinPath(tmpDir, 'out') await mkdir(outDir) - const context = makeContext({schema_path: 'src/schema.json'}) - const result = await copyConfigKeyEntry( - {key: 'schema_path', baseDir: tmpDir, outputDir: outDir, context}, - {stdout: mockStdout}, - ) - - expect(result).toBe(1) - await expect(fileExists(joinPath(outDir, 'schema.json'))).resolves.toBe(true) - const content = await readFile(joinPath(outDir, 'schema.json')) - expect(content).toBe('{}') - }) + // Then + expect(fs.copyFile).toHaveBeenCalledWith('/ext/src/schema.json', '/out/schema.json') + expect(result.filesCopied).toBe(1) + expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining("Copied 'src/schema.json' to schema.json")) }) test('skips with log message when configKey is absent from configuration', async () => { @@ -72,9 +81,10 @@ describe('copyConfigKeyEntry', () => { {stdout: mockStdout}, ) - expect(result).toBe(0) - expect(mockStdout.write).toHaveBeenCalledWith("No value for configKey 'static_root', skipping\n") - }) + // Then + expect(result.filesCopied).toBe(0) + expect(fs.fileExists).not.toHaveBeenCalled() + expect(mockStdout.write).toHaveBeenCalledWith("No value for configKey 'static_root', skipping\n") }) test('skips with warning when path resolved from config does not exist on disk', async () => { @@ -89,11 +99,10 @@ describe('copyConfigKeyEntry', () => { {stdout: mockStdout}, ) - expect(result).toBe(0) - expect(mockStdout.write).toHaveBeenCalledWith( - expect.stringContaining("Warning: path 'nonexistent' does not exist"), - ) - }) + // Then + expect(result.filesCopied).toBe(0) + expect(fs.copyDirectoryContents).not.toHaveBeenCalled() + expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining("Warning: path 'nonexistent' does not exist")) }) test('resolves array config value and copies each path, summing results', async () => { @@ -107,22 +116,10 @@ describe('copyConfigKeyEntry', () => { await mkdir(assetsDir) await writeFile(joinPath(assetsDir, 'logo.svg'), 'svg') - const outDir = joinPath(tmpDir, 'out') - await mkdir(outDir) - - const context = makeContext({roots: ['public', 'assets']}) - const result = await copyConfigKeyEntry( - {key: 'roots', baseDir: tmpDir, outputDir: outDir, context}, - {stdout: mockStdout}, - ) - - // Promise.all runs copies in parallel; glob on the shared outDir may see files - // from the other copy, so the total count is at least 3 (one per real file). - expect(result).toBeGreaterThanOrEqual(3) - await expect(fileExists(joinPath(outDir, 'a.html'))).resolves.toBe(true) - await expect(fileExists(joinPath(outDir, 'b.html'))).resolves.toBe(true) - await expect(fileExists(joinPath(outDir, 'logo.svg'))).resolves.toBe(true) - }) + // Then + expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/ext/public', '/out') + expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/ext/assets', '/out') + expect(result.filesCopied).toBe(4) }) test('prefixes outputDir with destination when destination param is provided', async () => { @@ -169,6 +166,23 @@ describe('copyConfigKeyEntry', () => { await expect(fileExists(joinPath(outDir, 'schema-b.json'))).resolves.toBe(true) await expect(fileExists(joinPath(outDir, 'schema-c.json'))).resolves.toBe(true) }) + // Source files exist; output paths are free so findUniqueDestPath resolves on first attempt + vi.mocked(fs.fileExists).mockImplementation(async (p) => String(p).startsWith('/ext/')) + vi.mocked(fs.isDirectory).mockResolvedValue(false) + vi.mocked(fs.mkdir).mockResolvedValue() + vi.mocked(fs.copyFile).mockResolvedValue() + + // When + const result = await copyConfigKeyEntry( + {key: 'extensions[].targeting[].schema', baseDir: '/ext', outputDir: '/out', context, preserveStructure: false}, + {stdout: mockStdout}, + ) + + // Then — all three schemas copied + expect(fs.copyFile).toHaveBeenCalledWith('/ext/schema-a.json', '/out/schema-a.json') + expect(fs.copyFile).toHaveBeenCalledWith('/ext/schema-b.json', '/out/schema-b.json') + expect(fs.copyFile).toHaveBeenCalledWith('/ext/schema-c.json', '/out/schema-c.json') + expect(result.filesCopied).toBe(3) }) test('skips with no-value log when [] flatten resolves to a non-array (contract violated)', async () => { @@ -190,5 +204,19 @@ describe('copyConfigKeyEntry', () => { expect.stringContaining("No value for configKey 'extensions[].targeting[].schema'"), ) }) + + // When + const result = await copyConfigKeyEntry( + {key: 'extensions[].targeting[].schema', baseDir: '/ext', outputDir: '/out', context, preserveStructure: false}, + {stdout: mockStdout}, + ) + + // Then — getNestedValue returns undefined, treated as absent key + expect(result.filesCopied).toBe(0) + expect(fs.copyDirectoryContents).not.toHaveBeenCalled() + expect(fs.copyFile).not.toHaveBeenCalled() + expect(mockStdout.write).toHaveBeenCalledWith( + expect.stringContaining("No value for configKey 'extensions[].targeting[].schema'"), + ) }) }) diff --git a/packages/app/src/cli/services/build/steps/include-assets/generate-manifest.ts b/packages/app/src/cli/services/build/steps/include-assets/generate-manifest.ts index 754cfaa42b9..1d6bbb63fc8 100644 --- a/packages/app/src/cli/services/build/steps/include-assets/generate-manifest.ts +++ b/packages/app/src/cli/services/build/steps/include-assets/generate-manifest.ts @@ -3,7 +3,7 @@ import {joinPath} from '@shopify/cli-kit/node/path' import {fileExists, mkdir, writeFile} from '@shopify/cli-kit/node/fs' import type {BuildContext} from '../../client-steps.js' -type ConfigKeyManifestEntry = { +interface ConfigKeyManifestEntry { anchor?: string | undefined groupBy?: string | undefined key: string From e81990d8e6584011ffedd455ae910ee4212aad25 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Wed, 1 Apr 2026 11:25:59 +0200 Subject: [PATCH 4/5] =?UTF-8?q?Address=20review:=20rename=20generateManife?= =?UTF-8?q?st=20=E2=86=92=20generatesAssetsManifest,=20all=20entries=20con?= =?UTF-8?q?tribute=20to=20manifest?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Rename generateManifest → generatesAssetsManifest per isaacroldan's suggestion - All entry types (pattern, static, configKey) now contribute their copied output paths to manifest.json when generatesAssetsManifest is true - pattern/static paths appear under a top-level "files" key - anchor/groupBy behavior for configKey entries unchanged - copyByPattern and copySourceEntry now return {filesCopied, outputPaths} - Add anchor/groupBy JSDoc to ConfigKeyEntrySchema Co-authored-by: Claude Sonnet 4.6 Co-authored-by: Claude Code --- .../build/steps/include-assets-step.test.ts | 78 +++++++++++++------ .../build/steps/include-assets-step.ts | 42 ++++++---- .../include-assets/copy-by-pattern.test.ts | 67 ++++++++++++++-- .../steps/include-assets/copy-by-pattern.ts | 24 +++--- .../copy-config-key-entry.test.ts | 4 +- .../include-assets/copy-source-entry.test.ts | 43 +++++++--- .../steps/include-assets/copy-source-entry.ts | 12 +-- .../steps/include-assets/generate-manifest.ts | 7 +- 8 files changed, 208 insertions(+), 69 deletions(-) diff --git a/packages/app/src/cli/services/build/steps/include-assets-step.test.ts b/packages/app/src/cli/services/build/steps/include-assets-step.test.ts index a3101a0498f..40490fe8496 100644 --- a/packages/app/src/cli/services/build/steps/include-assets-step.test.ts +++ b/packages/app/src/cli/services/build/steps/include-assets-step.test.ts @@ -697,7 +697,7 @@ describe('executeIncludeAssetsStep', () => { name: 'Generate Manifest', type: 'include_assets', config: { - generateManifest: true, + generatesAssetsManifest: true, inclusions: [ { type: 'configKey', @@ -753,7 +753,7 @@ describe('executeIncludeAssetsStep', () => { name: 'Generate Manifest', type: 'include_assets', config: { - generateManifest: true, + generatesAssetsManifest: true, inclusions: [ { type: 'configKey', @@ -817,7 +817,7 @@ describe('executeIncludeAssetsStep', () => { name: 'Generate Manifest', type: 'include_assets', config: { - generateManifest: true, + generatesAssetsManifest: true, inclusions: [ { type: 'configKey', @@ -854,7 +854,7 @@ describe('executeIncludeAssetsStep', () => { }) }) - test('does NOT write manifest.json when generateManifest is false (default)', async () => { + test('does NOT write manifest.json when generatesAssetsManifest is false (default)', async () => { // Given const contextWithConfig = { ...mockContext, @@ -872,7 +872,7 @@ describe('executeIncludeAssetsStep', () => { vi.mocked(fs.fileExists).mockResolvedValue(false) - // No generateManifest field — defaults to false + // No generatesAssetsManifest field — defaults to false const step: LifecycleStep = { id: 'gen-manifest', name: 'Generate Manifest', @@ -896,18 +896,20 @@ describe('executeIncludeAssetsStep', () => { expect(fs.writeFile).not.toHaveBeenCalled() }) - test('does NOT write manifest.json when generateManifest is true but all inclusions are pattern/static', async () => { - // Given — pattern and static entries never contribute to the manifest + test('writes manifest.json with files array when generatesAssetsManifest is true and only pattern inclusions exist', async () => { + // Given — pattern entries contribute output paths to the manifest "files" array vi.mocked(fs.glob).mockResolvedValue(['/test/extension/public/logo.png']) vi.mocked(fs.copyFile).mockResolvedValue() vi.mocked(fs.mkdir).mockResolvedValue() + vi.mocked(fs.fileExists).mockResolvedValue(false) + vi.mocked(fs.writeFile).mockResolvedValue() const step: LifecycleStep = { id: 'gen-manifest', name: 'Generate Manifest', type: 'include_assets', config: { - generateManifest: true, + generatesAssetsManifest: true, inclusions: [{type: 'pattern', baseDir: 'public', include: ['**/*']}], }, } @@ -915,8 +917,40 @@ describe('executeIncludeAssetsStep', () => { // When await executeIncludeAssetsStep(step, mockContext) - // Then — no configKey inclusions → no manifest written - expect(fs.writeFile).not.toHaveBeenCalled() + // Then — pattern entry contributes its output path to the manifest + expect(fs.writeFile).toHaveBeenCalledOnce() + const manifestContent = JSON.parse(vi.mocked(fs.writeFile).mock.calls[0]![1] as string) + expect(manifestContent).toEqual({files: ['logo.png']}) + }) + + test('writes manifest.json with files array from static entry when generatesAssetsManifest is true', async () => { + // Given — static file entry contributes its output path to the manifest "files" array + vi.mocked(fs.fileExists).mockResolvedValue(true) + vi.mocked(fs.isDirectory).mockResolvedValue(false) + vi.mocked(fs.mkdir).mockResolvedValue() + vi.mocked(fs.copyFile).mockResolvedValue() + vi.mocked(fs.writeFile).mockResolvedValue() + + // fileExists returns false for the manifest.json output path check + vi.mocked(fs.fileExists).mockImplementation(async (path) => String(path) !== '/test/output/manifest.json') + + const step: LifecycleStep = { + id: 'gen-manifest', + name: 'Generate Manifest', + type: 'include_assets', + config: { + generatesAssetsManifest: true, + inclusions: [{type: 'static', source: 'src/schema.json'}], + }, + } + + // When + await executeIncludeAssetsStep(step, mockContext) + + // Then — static entry contributes its output path to the manifest + expect(fs.writeFile).toHaveBeenCalledOnce() + const manifestContent = JSON.parse(vi.mocked(fs.writeFile).mock.calls[0]![1] as string) + expect(manifestContent).toEqual({files: ['schema.json']}) }) test('writes root-level manifest entry from non-anchored configKey inclusion', async () => { @@ -934,7 +968,7 @@ describe('executeIncludeAssetsStep', () => { name: 'Generate Manifest', type: 'include_assets', config: { - generateManifest: true, + generatesAssetsManifest: true, inclusions: [ {type: 'configKey', key: 'targeting.tools'}, {type: 'configKey', key: 'targeting.instructions'}, @@ -978,7 +1012,7 @@ describe('executeIncludeAssetsStep', () => { name: 'Copy Static', type: 'include_assets', config: { - generateManifest: true, + generatesAssetsManifest: true, // no destination, no preserveStructure → contents merged into output root inclusions: [{type: 'configKey', key: 'admin.static_root'}], }, @@ -1000,7 +1034,7 @@ describe('executeIncludeAssetsStep', () => { name: 'Generate Manifest', type: 'include_assets', config: { - generateManifest: true, + generatesAssetsManifest: true, inclusions: [{type: 'configKey', key: 'targeting.tools', anchor: 'targeting'}], }, } @@ -1036,7 +1070,7 @@ describe('executeIncludeAssetsStep', () => { name: 'Generate Manifest', type: 'include_assets', config: { - generateManifest: true, + generatesAssetsManifest: true, inclusions: [ { type: 'configKey', @@ -1072,7 +1106,7 @@ describe('executeIncludeAssetsStep', () => { name: 'Generate Manifest', type: 'include_assets', config: { - generateManifest: true, + generatesAssetsManifest: true, inclusions: [ { type: 'configKey', @@ -1116,7 +1150,7 @@ describe('executeIncludeAssetsStep', () => { name: 'Generate Manifest', type: 'include_assets', config: { - generateManifest: true, + generatesAssetsManifest: true, inclusions: [ { type: 'configKey', @@ -1161,7 +1195,7 @@ describe('executeIncludeAssetsStep', () => { name: 'Generate Manifest', type: 'include_assets', config: { - generateManifest: true, + generatesAssetsManifest: true, inclusions: [ { type: 'configKey', @@ -1182,7 +1216,7 @@ describe('executeIncludeAssetsStep', () => { expect(writeFileCall[0]).toBe('/test/output/manifest.json') }) - test('still copies files AND writes manifest when generateManifest is true', async () => { + test('still copies files AND writes manifest when generatesAssetsManifest is true', async () => { // Given const contextWithConfig = { ...mockContext, @@ -1201,7 +1235,7 @@ describe('executeIncludeAssetsStep', () => { name: 'Generate Manifest', type: 'include_assets', config: { - generateManifest: true, + generatesAssetsManifest: true, inclusions: [ { type: 'configKey', @@ -1246,7 +1280,7 @@ describe('executeIncludeAssetsStep', () => { name: 'Generate Manifest', type: 'include_assets', config: { - generateManifest: true, + generatesAssetsManifest: true, inclusions: [ { type: 'configKey', @@ -1293,7 +1327,7 @@ describe('executeIncludeAssetsStep', () => { name: 'Generate Manifest', type: 'include_assets', config: { - generateManifest: true, + generatesAssetsManifest: true, inclusions: [ { type: 'configKey', @@ -1349,7 +1383,7 @@ describe('executeIncludeAssetsStep', () => { name: 'Generate Manifest', type: 'include_assets', config: { - generateManifest: true, + generatesAssetsManifest: true, inclusions: [ { type: 'configKey', diff --git a/packages/app/src/cli/services/build/steps/include-assets-step.ts b/packages/app/src/cli/services/build/steps/include-assets-step.ts index 75c0f98c397..526186fb36f 100644 --- a/packages/app/src/cli/services/build/steps/include-assets-step.ts +++ b/packages/app/src/cli/services/build/steps/include-assets-step.ts @@ -39,8 +39,14 @@ const StaticEntrySchema = z.object({ * key is absent. Respects `preserveStructure` and `destination` the same way * as the static entry. * - * `anchor` and `groupBy` are optional fields used for manifest generation. - * When both are present, this entry participates in `generateManifestFile`. + * `anchor` — the config key path whose array value provides the grouping + * dimension. Each array item becomes one top-level manifest entry, keyed by + * its `groupBy` field value. + * + * `groupBy` — the field name within each anchor array item whose string value + * becomes the manifest key (e.g. `"target"` → `manifest["admin.link"] = {...}`). + * + * Both `anchor` and `groupBy` must be set together or both omitted. */ const ConfigKeyEntrySchema = z.object({ type: z.literal('configKey'), @@ -61,14 +67,16 @@ const InclusionEntrySchema = z.discriminatedUnion('type', [PatternEntrySchema, S * sequentially (to avoid filesystem race conditions on shared output paths), * then `pattern` and `static` entries run in parallel. * - * When `generateManifest` is `true`, a `manifest.json` file is written to the - * output directory after all inclusions complete. Only `configKey` entries - * that have both `anchor` and `groupBy` set participate in manifest generation. + * When `generatesAssetsManifest` is `true`, a `manifest.json` file is written + * to the output directory after all inclusions complete. All entry types + * contribute their copied output paths to the manifest. `configKey` entries + * with `anchor` and `groupBy` produce structured manifest entries; `pattern` + * and `static` entries contribute their paths under a `"files"` key. */ const IncludeAssetsConfigSchema = z .object({ inclusions: z.array(InclusionEntrySchema), - generateManifest: z.boolean().default(false), + generatesAssetsManifest: z.boolean().default(false), }) .superRefine((data, ctx) => { for (const [i, entry] of data.inclusions.entries()) { @@ -97,6 +105,9 @@ const IncludeAssetsConfigSchema = z * Runs sequentially to avoid filesystem race conditions. * - `type: 'pattern'` — glob-based file selection from a source directory * (defaults to extension root when `source` is omitted). + * + * When `generatesAssetsManifest` is `true`, all entry types contribute their + * copied output paths to `manifest.json`. */ export async function executeIncludeAssetsStep( step: LifecycleStep, @@ -135,9 +146,7 @@ export async function executeIncludeAssetsStep( configKeyCount += result.filesCopied } - // pattern and static entries do not use findUniqueDestPath and do not - // contribute to the pathMap, so they are safe to run in parallel. - const otherCounts = await Promise.all( + const otherResults = await Promise.all( config.inclusions .filter((entry) => entry.type !== 'configKey') .map(async (entry) => { @@ -148,7 +157,7 @@ export async function executeIncludeAssetsStep( if (entry.type === 'pattern') { const sourceDir = entry.baseDir ? joinPath(extension.directory, entry.baseDir) : extension.directory const destinationDir = sanitizedDest ? joinPath(outputDir, sanitizedDest) : outputDir - return copyByPattern( + const result = await copyByPattern( { sourceDir, outputDir: destinationDir, @@ -157,6 +166,11 @@ export async function executeIncludeAssetsStep( }, options, ) + // result.outputPaths are relative to destinationDir; prefix with sanitizedDest for outer outputDir relativity + const outputPaths = sanitizedDest + ? result.outputPaths.map((outputPath) => joinPath(sanitizedDest, outputPath)) + : result.outputPaths + return {filesCopied: result.filesCopied, outputPaths} } if (entry.type === 'static') { @@ -173,11 +187,13 @@ export async function executeIncludeAssetsStep( }), ) - const counts = [configKeyCount, ...otherCounts] + const otherFiles = config.generatesAssetsManifest ? otherResults.flatMap((result) => result?.outputPaths ?? []) : [] + + const counts = [configKeyCount, ...otherResults.map((result) => result?.filesCopied ?? 0)] - if (config.generateManifest) { + if (config.generatesAssetsManifest) { const configKeyEntries = config.inclusions.filter((entry) => entry.type === 'configKey') - await generateManifestFile(configKeyEntries, context, outputDir, aggregatedPathMap) + await generateManifestFile(configKeyEntries, context, outputDir, aggregatedPathMap, otherFiles) } return {filesCopied: counts.reduce((sum, count) => sum + (count ?? 0), 0)} diff --git a/packages/app/src/cli/services/build/steps/include-assets/copy-by-pattern.test.ts b/packages/app/src/cli/services/build/steps/include-assets/copy-by-pattern.test.ts index 7184f8f2912..527eddac717 100644 --- a/packages/app/src/cli/services/build/steps/include-assets/copy-by-pattern.test.ts +++ b/packages/app/src/cli/services/build/steps/include-assets/copy-by-pattern.test.ts @@ -31,8 +31,60 @@ describe('copyByPattern', () => { // Then expect(fs.copyFile).toHaveBeenCalledWith('/src/components/Button.tsx', '/out/components/Button.tsx') expect(fs.copyFile).toHaveBeenCalledWith('/src/utils/helpers.ts', '/out/utils/helpers.ts') - expect(result).toBe(2) - expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('Included 2 file(s)')) + expect(result.filesCopied).toBe(2) + expect(result.outputPaths).toEqual(expect.arrayContaining(['components/Button.tsx', 'utils/helpers.ts'])) + expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('Copied 2 file(s)')) + }) + + test('flattens files to basename when preserveStructure is false', async () => { + // Given + vi.mocked(fs.glob).mockResolvedValue(['/src/components/Button.tsx', '/src/utils/helper.ts']) + vi.mocked(fs.mkdir).mockResolvedValue() + vi.mocked(fs.copyFile).mockResolvedValue() + + // When + const result = await copyByPattern( + { + sourceDir: '/src', + outputDir: '/out', + patterns: ['**/*'], + ignore: [], + preserveStructure: false, + }, + {stdout: mockStdout}, + ) + + // Then + expect(fs.copyFile).toHaveBeenCalledWith('/src/components/Button.tsx', '/out/Button.tsx') + expect(fs.copyFile).toHaveBeenCalledWith('/src/utils/helper.ts', '/out/helper.ts') + expect(result.filesCopied).toBe(2) + expect(result.outputPaths).toEqual(expect.arrayContaining(['Button.tsx', 'helper.ts'])) + }) + + test('warns and lets last-in-array win when flattening produces basename collision', async () => { + // Given — two files with the same basename in different directories + vi.mocked(fs.glob).mockResolvedValue(['/src/a/index.js', '/src/b/index.js']) + vi.mocked(fs.mkdir).mockResolvedValue() + vi.mocked(fs.copyFile).mockResolvedValue() + + // When + const result = await copyByPattern( + { + sourceDir: '/src', + outputDir: '/out', + patterns: ['**/index.js'], + ignore: [], + preserveStructure: false, + }, + {stdout: mockStdout}, + ) + + // Then — collision warning emitted, only the last one is copied + expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('filename collision detected')) + expect(fs.copyFile).toHaveBeenCalledTimes(1) + expect(fs.copyFile).toHaveBeenCalledWith('/src/b/index.js', '/out/index.js') + expect(result.filesCopied).toBe(1) + expect(result.outputPaths).toEqual(['index.js']) }) test('warns and returns 0 when no files match patterns', async () => { @@ -51,7 +103,8 @@ describe('copyByPattern', () => { ) // Then - expect(result).toBe(0) + expect(result.filesCopied).toBe(0) + expect(result.outputPaths).toEqual([]) expect(fs.mkdir).not.toHaveBeenCalled() expect(fs.copyFile).not.toHaveBeenCalled() }) @@ -77,7 +130,8 @@ describe('copyByPattern', () => { // Then expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('skipping')) expect(fs.copyFile).not.toHaveBeenCalled() - expect(result).toBe(0) + expect(result.filesCopied).toBe(0) + expect(result.outputPaths).toEqual([]) }) test('returns 0 without copying when filepath equals computed destPath', async () => { @@ -99,8 +153,9 @@ describe('copyByPattern', () => { // Then expect(fs.copyFile).not.toHaveBeenCalled() - expect(result).toBe(0) - expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('Included 0 file(s)')) + expect(result.filesCopied).toBe(0) + expect(result.outputPaths).toEqual([]) + expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('Copied 0 file(s)')) }) test('calls mkdir(outputDir) before copying when files are matched', async () => { diff --git a/packages/app/src/cli/services/build/steps/include-assets/copy-by-pattern.ts b/packages/app/src/cli/services/build/steps/include-assets/copy-by-pattern.ts index 77a92d4d301..b6b0b8a5911 100644 --- a/packages/app/src/cli/services/build/steps/include-assets/copy-by-pattern.ts +++ b/packages/app/src/cli/services/build/steps/include-assets/copy-by-pattern.ts @@ -12,8 +12,8 @@ export async function copyByPattern( ignore: string[] }, options: {stdout: NodeJS.WritableStream}, -): Promise { - const {sourceDir, outputDir, patterns, ignore} = config +): Promise<{filesCopied: number; outputPaths: string[]}> { + const {sourceDir, outputDir, patterns, ignore, preserveStructure} = config const files = await glob(patterns, { absolute: true, cwd: sourceDir, @@ -21,7 +21,8 @@ export async function copyByPattern( }) if (files.length === 0) { - return 0 + options.stdout.write(`Warning: No files matched patterns in ${sourceDir}\n`) + return {filesCopied: 0, outputPaths: []} } await mkdir(outputDir) @@ -29,24 +30,25 @@ export async function copyByPattern( const filesToCopy = files const copyResults = await Promise.all( - filesToCopy.map(async (filepath): Promise => { - const relPath = relativePath(sourceDir, filepath) + filesToCopy.map(async (filepath): Promise<{count: number; path: string | null}> => { + const relPath = preserveStructure ? relativePath(sourceDir, filepath) : basename(filepath) const destPath = joinPath(outputDir, relPath) if (relativePath(outputDir, destPath).startsWith('..')) { options.stdout.write(`Warning: skipping '${filepath}' - resolved destination is outside the output directory\n`) - return 0 + return {count: 0, path: null} } - if (filepath === destPath) return 0 + if (filepath === destPath) return {count: 0, path: null} await mkdir(dirname(destPath)) await copyFile(filepath, destPath) - return 1 + return {count: 1, path: relPath} }), ) - const copiedCount = copyResults.reduce((sum, count) => sum + count, 0) - options.stdout.write(`Included ${copiedCount} file(s)\n`) - return copiedCount + const filesCopied = copyResults.reduce((sum, result) => sum + result.count, 0) + const outputPaths = copyResults.flatMap((result) => (result.path !== null ? [result.path] : [])) + options.stdout.write(`Copied ${filesCopied} file(s) from ${sourceDir} to ${outputDir}\n`) + return {filesCopied, outputPaths} } diff --git a/packages/app/src/cli/services/build/steps/include-assets/copy-config-key-entry.test.ts b/packages/app/src/cli/services/build/steps/include-assets/copy-config-key-entry.test.ts index b19afe0b78d..f25e8ad7d1c 100644 --- a/packages/app/src/cli/services/build/steps/include-assets/copy-config-key-entry.test.ts +++ b/packages/app/src/cli/services/build/steps/include-assets/copy-config-key-entry.test.ts @@ -56,7 +56,7 @@ describe('copyConfigKeyEntry', () => { // Given const context = makeContext({schema_path: 'src/schema.json'}) // Source file exists; output path is free so findUniqueDestPath resolves on first attempt - vi.mocked(fs.fileExists).mockImplementation(async (p) => String(p) === '/ext/src/schema.json') + vi.mocked(fs.fileExists).mockImplementation(async (path) => String(path) === '/ext/src/schema.json') vi.mocked(fs.isDirectory).mockResolvedValue(false) vi.mocked(fs.mkdir).mockResolvedValue() vi.mocked(fs.copyFile).mockResolvedValue() @@ -167,7 +167,7 @@ describe('copyConfigKeyEntry', () => { await expect(fileExists(joinPath(outDir, 'schema-c.json'))).resolves.toBe(true) }) // Source files exist; output paths are free so findUniqueDestPath resolves on first attempt - vi.mocked(fs.fileExists).mockImplementation(async (p) => String(p).startsWith('/ext/')) + vi.mocked(fs.fileExists).mockImplementation(async (path) => String(path).startsWith('/ext/')) vi.mocked(fs.isDirectory).mockResolvedValue(false) vi.mocked(fs.mkdir).mockResolvedValue() vi.mocked(fs.copyFile).mockResolvedValue() diff --git a/packages/app/src/cli/services/build/steps/include-assets/copy-source-entry.test.ts b/packages/app/src/cli/services/build/steps/include-assets/copy-source-entry.test.ts index 889b7a626c0..b87809ab807 100644 --- a/packages/app/src/cli/services/build/steps/include-assets/copy-source-entry.test.ts +++ b/packages/app/src/cli/services/build/steps/include-assets/copy-source-entry.test.ts @@ -49,8 +49,9 @@ describe('copySourceEntry', () => { // Then expect(fs.copyFile).toHaveBeenCalledWith('/ext/src/icon.png', '/out/assets/icon.png') - expect(result).toBe(1) - expect(mockStdout.write).toHaveBeenCalledWith('Included src/icon.png\n') + expect(result.filesCopied).toBe(1) + expect(result.outputPaths).toEqual(['assets/icon.png']) + expect(mockStdout.write).toHaveBeenCalledWith('Copied src/icon.png to assets/icon.png\n') }) test('copies directory under its own name when no destination is given', async () => { @@ -68,8 +69,29 @@ describe('copySourceEntry', () => { // Then expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/ext/dist', '/out/dist') - expect(result).toBe(2) - expect(mockStdout.write).toHaveBeenCalledWith('Included dist\n') + expect(result.filesCopied).toBe(2) + expect(result.outputPaths).toEqual(['dist/index.html', 'dist/logo.png']) + expect(mockStdout.write).toHaveBeenCalledWith('Copied dist to dist\n') + }) + + test('merges directory contents into output root when no destination and preserveStructure is false', async () => { + // Given + vi.mocked(fs.fileExists).mockResolvedValue(true) + vi.mocked(fs.isDirectory).mockResolvedValue(true) + vi.mocked(fs.copyDirectoryContents).mockResolvedValue() + vi.mocked(fs.glob).mockResolvedValue(['a.js', 'b.js', 'c.js']) + + // When + const result = await copySourceEntry( + {source: 'build', destination: undefined, baseDir: '/ext', outputDir: '/out', preserveStructure: false}, + {stdout: mockStdout}, + ) + + // Then + expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/ext/build', '/out') + expect(result.filesCopied).toBe(3) + expect(result.outputPaths).toEqual(['a.js', 'b.js', 'c.js']) + expect(mockStdout.write).toHaveBeenCalledWith('Copied contents of build to output root\n') }) test('copies file to basename in outputDir when source is a file and no destination given', async () => { @@ -87,8 +109,9 @@ describe('copySourceEntry', () => { // Then expect(fs.copyFile).toHaveBeenCalledWith('/ext/README.md', '/out/README.md') - expect(result).toBe(1) - expect(mockStdout.write).toHaveBeenCalledWith('Included README.md\n') + expect(result.filesCopied).toBe(1) + expect(result.outputPaths).toEqual(['README.md']) + expect(mockStdout.write).toHaveBeenCalledWith('Copied README.md to README.md\n') }) test('copies directory to explicit destination path', async () => { @@ -106,8 +129,9 @@ describe('copySourceEntry', () => { // Then expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/ext/dist', '/out/vendor/dist') - expect(result).toBe(1) - expect(mockStdout.write).toHaveBeenCalledWith('Included dist\n') + expect(result.filesCopied).toBe(1) + expect(result.outputPaths).toEqual(['vendor/dist/x.js']) + expect(mockStdout.write).toHaveBeenCalledWith('Copied dist to vendor/dist\n') }) test('returns count of files discovered in destination directory after directory copy', async () => { @@ -125,7 +149,8 @@ describe('copySourceEntry', () => { ) // Then — count comes from glob on destPath, not a constant - expect(result).toBe(5) + expect(result.filesCopied).toBe(5) + expect(result.outputPaths).toEqual(['a.js', 'b.js', 'c.js', 'd.js', 'e.js']) }) test('creates parent directories before copying a file', async () => { diff --git a/packages/app/src/cli/services/build/steps/include-assets/copy-source-entry.ts b/packages/app/src/cli/services/build/steps/include-assets/copy-source-entry.ts index 81ae331f926..6499ba32da2 100644 --- a/packages/app/src/cli/services/build/steps/include-assets/copy-source-entry.ts +++ b/packages/app/src/cli/services/build/steps/include-assets/copy-source-entry.ts @@ -1,4 +1,4 @@ -import {joinPath, dirname, basename} from '@shopify/cli-kit/node/path' +import {joinPath, dirname, basename, relativePath} from '@shopify/cli-kit/node/path' import {glob, copyFile, copyDirectoryContents, fileExists, mkdir, isDirectory} from '@shopify/cli-kit/node/fs' /** @@ -15,8 +15,8 @@ export async function copySourceEntry( outputDir: string }, options: {stdout: NodeJS.WritableStream}, -): Promise { - const {source, destination, baseDir, outputDir} = config +): Promise<{filesCopied: number; outputPaths: string[]}> { + const {source, destination, baseDir, outputDir, preserveStructure} = config const sourcePath = joinPath(baseDir, source) if (!(await fileExists(sourcePath))) { throw new Error(`Source does not exist: ${sourcePath}`) @@ -39,11 +39,13 @@ export async function copySourceEntry( await copyDirectoryContents(sourcePath, destPath) const copied = await glob(['**/*'], {cwd: destPath, absolute: false}) options.stdout.write(logMsg) - return copied.length + const destRelToOutput = relativePath(outputDir, destPath) + const outputPaths = destRelToOutput ? copied.map((file) => joinPath(destRelToOutput, file)) : copied + return {filesCopied: copied.length, outputPaths} } await mkdir(dirname(destPath)) await copyFile(sourcePath, destPath) options.stdout.write(logMsg) - return 1 + return {filesCopied: 1, outputPaths: [relativePath(outputDir, destPath)]} } diff --git a/packages/app/src/cli/services/build/steps/include-assets/generate-manifest.ts b/packages/app/src/cli/services/build/steps/include-assets/generate-manifest.ts index 1d6bbb63fc8..9aad570ee99 100644 --- a/packages/app/src/cli/services/build/steps/include-assets/generate-manifest.ts +++ b/packages/app/src/cli/services/build/steps/include-assets/generate-manifest.ts @@ -29,6 +29,7 @@ export async function generateManifestFile( context: BuildContext, outputDir: string, pathMap: Map, + otherFiles: string[], ): Promise { const {extension, options} = context @@ -45,7 +46,7 @@ export async function generateManifestFile( } } - if (anchoredIncs.length === 0 && rootIncs.length === 0) return + if (anchoredIncs.length === 0 && rootIncs.length === 0 && otherFiles.length === 0) return const manifest: {[key: string]: unknown} = {} @@ -101,6 +102,10 @@ export async function generateManifestFile( } } + if (otherFiles.length > 0) { + manifest.files = otherFiles + } + if (Object.keys(manifest).length === 0) { options.stdout.write('Warning: no manifest entries produced — skipping manifest.json\n') return From f042d96ce3093d398a80db3752225da5f7c30874 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Wed, 1 Apr 2026 11:26:00 +0200 Subject: [PATCH 5/5] Remove preserveStructure from manifest branch, fix log messages and tests - Remove `preserveStructure` field from ConfigKeyEntrySchema and all call sites - copy-by-pattern: always use relativePath (was the default=true behaviour); remove no-match warning; update log to `Included N file(s)` - copy-config-key-entry: remove preserveStructure branching; update log to `Included '...'` - copy-source-entry: remove stale preserveStructure destructure - Tests: rewrite copy-config-key-entry.test.ts (was broken from conflict resolution); delete removed-feature tests in copy-by-pattern and copy-source-entry; update log message assertions; fix outputPaths prefix in copy-source-entry test Co-authored-by: Claude Sonnet 4.6 Co-authored-by: Claude Code --- .../cli/models/extensions/specification.ts | 1 + .../models/extensions/specifications/admin.ts | 2 +- .../cli/services/build/client-steps.test.ts | 3 +- .../src/cli/services/build/client-steps.ts | 38 ++-- .../build/steps/include-assets-step.test.ts | 4 +- .../build/steps/include-assets-step.ts | 14 +- .../include-assets/copy-by-pattern.test.ts | 57 +----- .../steps/include-assets/copy-by-pattern.ts | 9 +- .../copy-config-key-entry.test.ts | 178 ++++++++++-------- .../include-assets/copy-config-key-entry.ts | 12 +- .../include-assets/copy-source-entry.test.ts | 30 +-- .../steps/include-assets/copy-source-entry.ts | 2 +- 12 files changed, 158 insertions(+), 192 deletions(-) diff --git a/packages/app/src/cli/models/extensions/specification.ts b/packages/app/src/cli/models/extensions/specification.ts index e4df88b5b8d..2f7b1bea33c 100644 --- a/packages/app/src/cli/models/extensions/specification.ts +++ b/packages/app/src/cli/models/extensions/specification.ts @@ -302,6 +302,7 @@ export function createContractBasedModuleSpecification { diff --git a/packages/app/src/cli/models/extensions/specifications/admin.ts b/packages/app/src/cli/models/extensions/specifications/admin.ts index b7aa03027d2..0aea2ee661a 100644 --- a/packages/app/src/cli/models/extensions/specifications/admin.ts +++ b/packages/app/src/cli/models/extensions/specifications/admin.ts @@ -24,7 +24,7 @@ const adminSpecificationSpec = createContractBasedModuleSpecification({ name: 'Hosted App Copy Files', type: 'include_assets', config: { - generateManifest: true, + generatesAssetsManifest: true, inclusions: [ { type: 'configKey', diff --git a/packages/app/src/cli/services/build/client-steps.test.ts b/packages/app/src/cli/services/build/client-steps.test.ts index 3c33e94891b..ec7b4f9c671 100644 --- a/packages/app/src/cli/services/build/client-steps.test.ts +++ b/packages/app/src/cli/services/build/client-steps.test.ts @@ -27,8 +27,7 @@ describe('executeStep', () => { const step: LifecycleStep = { id: 'test-step', name: 'Test Step', - type: 'include_assets', - config: {}, + type: 'build_theme', } describe('success', () => { diff --git a/packages/app/src/cli/services/build/client-steps.ts b/packages/app/src/cli/services/build/client-steps.ts index b84b6bbc35a..94fedfaf31c 100644 --- a/packages/app/src/cli/services/build/client-steps.ts +++ b/packages/app/src/cli/services/build/client-steps.ts @@ -1,35 +1,47 @@ import {executeStepByType} from './steps/index.js' +import type {IncludeAssetsConfig} from './steps/include-assets-step.js' import type {ExtensionInstance} from '../../models/extensions/extension-instance.js' import type {ExtensionBuildOptions} from './extension.js' -/** - * LifecycleStep represents a single step in the client-side build pipeline. - * Pure configuration object — execution logic is separate (router pattern). - */ -export interface LifecycleStep { +/** Common fields shared by all lifecycle steps. */ +interface BaseStep { /** Unique identifier, used as the key in the stepResults map */ readonly id: string /** Human-readable name for logging */ readonly name: string - /** Step type (determines which executor handles it) */ + /** Whether to continue on error (default: false) */ + readonly continueOnError?: boolean +} + +/** Step with typed config specific to include_assets. */ +interface IncludeAssetsStep extends BaseStep { + readonly type: 'include_assets' + readonly config: IncludeAssetsConfig +} + +/** Steps that don't require any config yet. */ +interface NoConfigStep extends BaseStep { readonly type: - | 'include_assets' | 'build_theme' | 'bundle_theme' | 'bundle_ui' | 'copy_static_assets' | 'build_function' | 'create_tax_stub' - - /** Step-specific configuration */ - readonly config: {[key: string]: unknown} - - /** Whether to continue on error (default: false) */ - readonly continueOnError?: boolean + readonly config?: Record } +/** + * LifecycleStep represents a single step in the client-side build pipeline. + * Pure configuration object — execution logic is separate (router pattern). + * + * This is a discriminated union on `type`: each step type carries its own + * typed `config`, so TypeScript catches config typos at compile time. + */ +export type LifecycleStep = IncludeAssetsStep | NoConfigStep + /** * A group of steps scoped to a specific lifecycle phase. * Allows executing only the steps relevant to a given lifecycle (e.g. 'deploy'). diff --git a/packages/app/src/cli/services/build/steps/include-assets-step.test.ts b/packages/app/src/cli/services/build/steps/include-assets-step.test.ts index 40490fe8496..da019d8d22a 100644 --- a/packages/app/src/cli/services/build/steps/include-assets-step.test.ts +++ b/packages/app/src/cli/services/build/steps/include-assets-step.test.ts @@ -195,7 +195,7 @@ describe('executeIncludeAssetsStep', () => { expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/test/extension/dist', '/test/output/assets/dist') expect(fs.copyFile).not.toHaveBeenCalled() expect(result.filesCopied).toBe(3) - expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('Copied dist to assets/dist')) + expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('Included dist')) }) }) @@ -1013,7 +1013,7 @@ describe('executeIncludeAssetsStep', () => { type: 'include_assets', config: { generatesAssetsManifest: true, - // no destination, no preserveStructure → contents merged into output root + // no destination → contents merged into output root inclusions: [{type: 'configKey', key: 'admin.static_root'}], }, } diff --git a/packages/app/src/cli/services/build/steps/include-assets-step.ts b/packages/app/src/cli/services/build/steps/include-assets-step.ts index 526186fb36f..e3a5b9a01b0 100644 --- a/packages/app/src/cli/services/build/steps/include-assets-step.ts +++ b/packages/app/src/cli/services/build/steps/include-assets-step.ts @@ -6,6 +6,8 @@ import {joinPath, dirname, extname, sanitizeRelativePath} from '@shopify/cli-kit import {z} from 'zod' import type {LifecycleStep, BuildContext} from '../client-steps.js' +export type {IncludeAssetsConfig} + /** * Pattern inclusion entry. * @@ -36,8 +38,7 @@ const StaticEntrySchema = z.object({ * * Resolves a path (or array of paths) from the extension configuration and * copies the directory contents into the output. Silently skipped when the - * key is absent. Respects `preserveStructure` and `destination` the same way - * as the static entry. + * key is absent. * * `anchor` — the config key path whose array value provides the grouping * dimension. Each array item becomes one top-level manifest entry, keyed by @@ -52,7 +53,6 @@ const ConfigKeyEntrySchema = z.object({ type: z.literal('configKey'), key: z.string(), destination: z.string().optional(), - preserveStructure: z.boolean().default(false), anchor: z.string().optional(), groupBy: z.string().optional(), }) @@ -78,6 +78,7 @@ const IncludeAssetsConfigSchema = z inclusions: z.array(InclusionEntrySchema), generatesAssetsManifest: z.boolean().default(false), }) + .strict() .superRefine((data, ctx) => { for (const [i, entry] of data.inclusions.entries()) { if (entry.type === 'configKey') { @@ -94,6 +95,13 @@ const IncludeAssetsConfigSchema = z } }) +/** + * TypeScript type for the include_assets step config. + * Derived from the Zod schema so that static type-checking catches + * mismatched property names at compile time. + */ +type IncludeAssetsConfig = z.input + /** * Executes an include_assets build step. * diff --git a/packages/app/src/cli/services/build/steps/include-assets/copy-by-pattern.test.ts b/packages/app/src/cli/services/build/steps/include-assets/copy-by-pattern.test.ts index 527eddac717..ea4cf855116 100644 --- a/packages/app/src/cli/services/build/steps/include-assets/copy-by-pattern.test.ts +++ b/packages/app/src/cli/services/build/steps/include-assets/copy-by-pattern.test.ts @@ -33,61 +33,10 @@ describe('copyByPattern', () => { expect(fs.copyFile).toHaveBeenCalledWith('/src/utils/helpers.ts', '/out/utils/helpers.ts') expect(result.filesCopied).toBe(2) expect(result.outputPaths).toEqual(expect.arrayContaining(['components/Button.tsx', 'utils/helpers.ts'])) - expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('Copied 2 file(s)')) + expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('Included 2 file(s)')) }) - test('flattens files to basename when preserveStructure is false', async () => { - // Given - vi.mocked(fs.glob).mockResolvedValue(['/src/components/Button.tsx', '/src/utils/helper.ts']) - vi.mocked(fs.mkdir).mockResolvedValue() - vi.mocked(fs.copyFile).mockResolvedValue() - - // When - const result = await copyByPattern( - { - sourceDir: '/src', - outputDir: '/out', - patterns: ['**/*'], - ignore: [], - preserveStructure: false, - }, - {stdout: mockStdout}, - ) - - // Then - expect(fs.copyFile).toHaveBeenCalledWith('/src/components/Button.tsx', '/out/Button.tsx') - expect(fs.copyFile).toHaveBeenCalledWith('/src/utils/helper.ts', '/out/helper.ts') - expect(result.filesCopied).toBe(2) - expect(result.outputPaths).toEqual(expect.arrayContaining(['Button.tsx', 'helper.ts'])) - }) - - test('warns and lets last-in-array win when flattening produces basename collision', async () => { - // Given — two files with the same basename in different directories - vi.mocked(fs.glob).mockResolvedValue(['/src/a/index.js', '/src/b/index.js']) - vi.mocked(fs.mkdir).mockResolvedValue() - vi.mocked(fs.copyFile).mockResolvedValue() - - // When - const result = await copyByPattern( - { - sourceDir: '/src', - outputDir: '/out', - patterns: ['**/index.js'], - ignore: [], - preserveStructure: false, - }, - {stdout: mockStdout}, - ) - - // Then — collision warning emitted, only the last one is copied - expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('filename collision detected')) - expect(fs.copyFile).toHaveBeenCalledTimes(1) - expect(fs.copyFile).toHaveBeenCalledWith('/src/b/index.js', '/out/index.js') - expect(result.filesCopied).toBe(1) - expect(result.outputPaths).toEqual(['index.js']) - }) - - test('warns and returns 0 when no files match patterns', async () => { + test('returns 0 when no files match patterns', async () => { // Given vi.mocked(fs.glob).mockResolvedValue([]) @@ -155,7 +104,7 @@ describe('copyByPattern', () => { expect(fs.copyFile).not.toHaveBeenCalled() expect(result.filesCopied).toBe(0) expect(result.outputPaths).toEqual([]) - expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('Copied 0 file(s)')) + expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('Included 0 file(s)')) }) test('calls mkdir(outputDir) before copying when files are matched', async () => { diff --git a/packages/app/src/cli/services/build/steps/include-assets/copy-by-pattern.ts b/packages/app/src/cli/services/build/steps/include-assets/copy-by-pattern.ts index b6b0b8a5911..c1716d7d2da 100644 --- a/packages/app/src/cli/services/build/steps/include-assets/copy-by-pattern.ts +++ b/packages/app/src/cli/services/build/steps/include-assets/copy-by-pattern.ts @@ -13,7 +13,7 @@ export async function copyByPattern( }, options: {stdout: NodeJS.WritableStream}, ): Promise<{filesCopied: number; outputPaths: string[]}> { - const {sourceDir, outputDir, patterns, ignore, preserveStructure} = config + const {sourceDir, outputDir, patterns, ignore} = config const files = await glob(patterns, { absolute: true, cwd: sourceDir, @@ -21,7 +21,6 @@ export async function copyByPattern( }) if (files.length === 0) { - options.stdout.write(`Warning: No files matched patterns in ${sourceDir}\n`) return {filesCopied: 0, outputPaths: []} } @@ -31,7 +30,7 @@ export async function copyByPattern( const copyResults = await Promise.all( filesToCopy.map(async (filepath): Promise<{count: number; path: string | null}> => { - const relPath = preserveStructure ? relativePath(sourceDir, filepath) : basename(filepath) + const relPath = relativePath(sourceDir, filepath) const destPath = joinPath(outputDir, relPath) if (relativePath(outputDir, destPath).startsWith('..')) { @@ -48,7 +47,7 @@ export async function copyByPattern( ) const filesCopied = copyResults.reduce((sum, result) => sum + result.count, 0) - const outputPaths = copyResults.flatMap((result) => (result.path !== null ? [result.path] : [])) - options.stdout.write(`Copied ${filesCopied} file(s) from ${sourceDir} to ${outputDir}\n`) + const outputPaths = copyResults.flatMap((result) => (result.path === null ? [] : [result.path])) + options.stdout.write(`Included ${filesCopied} file(s)\n`) return {filesCopied, outputPaths} } diff --git a/packages/app/src/cli/services/build/steps/include-assets/copy-config-key-entry.test.ts b/packages/app/src/cli/services/build/steps/include-assets/copy-config-key-entry.test.ts index f25e8ad7d1c..340788450ea 100644 --- a/packages/app/src/cli/services/build/steps/include-assets/copy-config-key-entry.test.ts +++ b/packages/app/src/cli/services/build/steps/include-assets/copy-config-key-entry.test.ts @@ -26,48 +26,65 @@ describe('copyConfigKeyEntry', () => { const outDir = joinPath(tmpDir, 'out') await mkdir(outDir) - // Then - expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/ext/public', '/out') - expect(result.filesCopied).toBe(2) - expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining('Copied contents of')) - }) + const context = makeContext({static_root: 'public'}) + const result = await copyConfigKeyEntry( + {key: 'static_root', baseDir: tmpDir, outputDir: outDir, context}, + {stdout: mockStdout}, + ) - test('places directory under its own name when preserveStructure is true', async () => { - // Given - const context = makeContext({theme_root: 'theme'}) - vi.mocked(fs.fileExists).mockResolvedValue(true) - vi.mocked(fs.isDirectory).mockResolvedValue(true) - vi.mocked(fs.copyDirectoryContents).mockResolvedValue() - vi.mocked(fs.glob).mockResolvedValue(['style.css', 'layout.liquid']) - - // When - const result = await copyConfigKeyEntry( - {key: 'theme_root', baseDir: '/ext', outputDir: '/out', context, preserveStructure: true}, - {stdout: mockStdout}, - ) - - // Then - expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/ext/theme', '/out/theme') - expect(result.filesCopied).toBe(2) - expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining("Copied 'theme' to theme")) + expect(result.filesCopied).toBe(2) + await expect(fileExists(joinPath(outDir, 'index.html'))).resolves.toBe(true) + await expect(fileExists(joinPath(outDir, 'logo.png'))).resolves.toBe(true) + expect(result.pathMap.get('public')).toEqual(expect.arrayContaining(['index.html', 'logo.png'])) + expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining("Included 'public'")) + }) }) test('copies a file source to outputDir/basename', async () => { - // Given - const context = makeContext({schema_path: 'src/schema.json'}) - // Source file exists; output path is free so findUniqueDestPath resolves on first attempt - vi.mocked(fs.fileExists).mockImplementation(async (path) => String(path) === '/ext/src/schema.json') - vi.mocked(fs.isDirectory).mockResolvedValue(false) - vi.mocked(fs.mkdir).mockResolvedValue() - vi.mocked(fs.copyFile).mockResolvedValue() + await inTemporaryDirectory(async (tmpDir) => { + const srcDir = joinPath(tmpDir, 'src') + await mkdir(srcDir) + await writeFile(joinPath(srcDir, 'schema.json'), '{}') const outDir = joinPath(tmpDir, 'out') await mkdir(outDir) - // Then - expect(fs.copyFile).toHaveBeenCalledWith('/ext/src/schema.json', '/out/schema.json') - expect(result.filesCopied).toBe(1) - expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining("Copied 'src/schema.json' to schema.json")) + const context = makeContext({schema_path: 'src/schema.json'}) + const result = await copyConfigKeyEntry( + {key: 'schema_path', baseDir: tmpDir, outputDir: outDir, context}, + {stdout: mockStdout}, + ) + + expect(result.filesCopied).toBe(1) + await expect(fileExists(joinPath(outDir, 'schema.json'))).resolves.toBe(true) + const content = await readFile(joinPath(outDir, 'schema.json')) + expect(content).toBe('{}') + expect(result.pathMap.get('src/schema.json')).toBe('schema.json') + expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining("Included 'src/schema.json'")) + }) + }) + + test('renames output file to avoid collision when candidate path already exists', async () => { + await inTemporaryDirectory(async (tmpDir) => { + await writeFile(joinPath(tmpDir, 'tools-a.json'), '{}') + await writeFile(joinPath(tmpDir, 'tools-b.json'), '{}') + + const outDir = joinPath(tmpDir, 'out') + await mkdir(outDir) + // Pre-create the first candidate to force a rename + await writeFile(joinPath(outDir, 'tools-a.json'), 'existing') + + const context = makeContext({files: ['tools-a.json', 'tools-b.json']}) + const result = await copyConfigKeyEntry( + {key: 'files', baseDir: tmpDir, outputDir: outDir, context}, + {stdout: mockStdout}, + ) + + expect(result.filesCopied).toBe(2) + // tools-a.json was taken, so the copy lands as tools-a-1.json + await expect(fileExists(joinPath(outDir, 'tools-a-1.json'))).resolves.toBe(true) + await expect(fileExists(joinPath(outDir, 'tools-b.json'))).resolves.toBe(true) + }) }) test('skips with log message when configKey is absent from configuration', async () => { @@ -81,10 +98,10 @@ describe('copyConfigKeyEntry', () => { {stdout: mockStdout}, ) - // Then - expect(result.filesCopied).toBe(0) - expect(fs.fileExists).not.toHaveBeenCalled() - expect(mockStdout.write).toHaveBeenCalledWith("No value for configKey 'static_root', skipping\n") + expect(result.filesCopied).toBe(0) + expect(result.pathMap.size).toBe(0) + expect(mockStdout.write).toHaveBeenCalledWith("No value for configKey 'static_root', skipping\n") + }) }) test('skips with warning when path resolved from config does not exist on disk', async () => { @@ -99,10 +116,12 @@ describe('copyConfigKeyEntry', () => { {stdout: mockStdout}, ) - // Then - expect(result.filesCopied).toBe(0) - expect(fs.copyDirectoryContents).not.toHaveBeenCalled() - expect(mockStdout.write).toHaveBeenCalledWith(expect.stringContaining("Warning: path 'nonexistent' does not exist")) + expect(result.filesCopied).toBe(0) + expect(result.pathMap.size).toBe(0) + expect(mockStdout.write).toHaveBeenCalledWith( + expect.stringContaining("Warning: path 'nonexistent' does not exist"), + ) + }) }) test('resolves array config value and copies each path, summing results', async () => { @@ -116,10 +135,22 @@ describe('copyConfigKeyEntry', () => { await mkdir(assetsDir) await writeFile(joinPath(assetsDir, 'logo.svg'), 'svg') - // Then - expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/ext/public', '/out') - expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/ext/assets', '/out') - expect(result.filesCopied).toBe(4) + const outDir = joinPath(tmpDir, 'out') + await mkdir(outDir) + + const context = makeContext({roots: ['public', 'assets']}) + const result = await copyConfigKeyEntry( + {key: 'roots', baseDir: tmpDir, outputDir: outDir, context}, + {stdout: mockStdout}, + ) + + // Promise.all runs copies sequentially; glob on the shared outDir may see files + // from the other copy, so the total count is at least 3 (one per real file). + expect(result.filesCopied).toBeGreaterThanOrEqual(3) + await expect(fileExists(joinPath(outDir, 'a.html'))).resolves.toBe(true) + await expect(fileExists(joinPath(outDir, 'b.html'))).resolves.toBe(true) + await expect(fileExists(joinPath(outDir, 'logo.svg'))).resolves.toBe(true) + }) }) test('prefixes outputDir with destination when destination param is provided', async () => { @@ -161,28 +192,11 @@ describe('copyConfigKeyEntry', () => { {stdout: mockStdout}, ) - expect(result).toBe(3) + expect(result.filesCopied).toBe(3) await expect(fileExists(joinPath(outDir, 'schema-a.json'))).resolves.toBe(true) await expect(fileExists(joinPath(outDir, 'schema-b.json'))).resolves.toBe(true) await expect(fileExists(joinPath(outDir, 'schema-c.json'))).resolves.toBe(true) }) - // Source files exist; output paths are free so findUniqueDestPath resolves on first attempt - vi.mocked(fs.fileExists).mockImplementation(async (path) => String(path).startsWith('/ext/')) - vi.mocked(fs.isDirectory).mockResolvedValue(false) - vi.mocked(fs.mkdir).mockResolvedValue() - vi.mocked(fs.copyFile).mockResolvedValue() - - // When - const result = await copyConfigKeyEntry( - {key: 'extensions[].targeting[].schema', baseDir: '/ext', outputDir: '/out', context, preserveStructure: false}, - {stdout: mockStdout}, - ) - - // Then — all three schemas copied - expect(fs.copyFile).toHaveBeenCalledWith('/ext/schema-a.json', '/out/schema-a.json') - expect(fs.copyFile).toHaveBeenCalledWith('/ext/schema-b.json', '/out/schema-b.json') - expect(fs.copyFile).toHaveBeenCalledWith('/ext/schema-c.json', '/out/schema-c.json') - expect(result.filesCopied).toBe(3) }) test('skips with no-value log when [] flatten resolves to a non-array (contract violated)', async () => { @@ -199,24 +213,32 @@ describe('copyConfigKeyEntry', () => { {stdout: mockStdout}, ) - expect(result).toBe(0) + expect(result.filesCopied).toBe(0) + expect(result.pathMap.size).toBe(0) expect(mockStdout.write).toHaveBeenCalledWith( expect.stringContaining("No value for configKey 'extensions[].targeting[].schema'"), ) }) + }) - // When - const result = await copyConfigKeyEntry( - {key: 'extensions[].targeting[].schema', baseDir: '/ext', outputDir: '/out', context, preserveStructure: false}, - {stdout: mockStdout}, - ) - - // Then — getNestedValue returns undefined, treated as absent key - expect(result.filesCopied).toBe(0) - expect(fs.copyDirectoryContents).not.toHaveBeenCalled() - expect(fs.copyFile).not.toHaveBeenCalled() - expect(mockStdout.write).toHaveBeenCalledWith( - expect.stringContaining("No value for configKey 'extensions[].targeting[].schema'"), - ) + test('deduplicates repeated source paths — copies each unique path only once', async () => { + await inTemporaryDirectory(async (tmpDir) => { + await writeFile(joinPath(tmpDir, 'tools.json'), '{}') + + const outDir = joinPath(tmpDir, 'out') + await mkdir(outDir) + + // Two items referencing the same file; should only be copied once + const context = makeContext({ + extensions: [{targeting: [{tools: 'tools.json'}, {tools: 'tools.json'}]}], + }) + const result = await copyConfigKeyEntry( + {key: 'extensions[].targeting[].tools', baseDir: tmpDir, outputDir: outDir, context}, + {stdout: mockStdout}, + ) + + expect(result.filesCopied).toBe(1) + await expect(fileExists(joinPath(outDir, 'tools.json'))).resolves.toBe(true) + }) }) }) diff --git a/packages/app/src/cli/services/build/steps/include-assets/copy-config-key-entry.ts b/packages/app/src/cli/services/build/steps/include-assets/copy-config-key-entry.ts index 9b400a22dd0..9c07fe97996 100644 --- a/packages/app/src/cli/services/build/steps/include-assets/copy-config-key-entry.ts +++ b/packages/app/src/cli/services/build/steps/include-assets/copy-config-key-entry.ts @@ -29,7 +29,7 @@ export async function copyConfigKeyEntry( }, options: {stdout: NodeJS.WritableStream}, ): Promise<{filesCopied: number; pathMap: Map}> { - const {key, baseDir, outputDir, context, preserveStructure, destination} = config + const {key, baseDir, outputDir, context, destination} = config const value = getNestedValue(context.extension.configuration, key) let paths: string[] if (typeof value === 'string') { @@ -67,16 +67,12 @@ export async function copyConfigKeyEntry( const sourceIsDir = await isDirectory(fullPath) - const destDir = - sourceIsDir && preserveStructure ? joinPath(effectiveOutputDir, basename(fullPath)) : effectiveOutputDir + const destDir = effectiveOutputDir if (sourceIsDir) { await copyDirectoryContents(fullPath, destDir) const copied = await glob(['**/*'], {cwd: destDir, absolute: false}) - const msg = preserveStructure - ? `Copied '${sourcePath}' to ${basename(fullPath)}\n` - : `Copied contents of '${sourcePath}' to output root\n` - options.stdout.write(msg) + options.stdout.write(`Included '${sourcePath}'\n`) const relFiles = copied.map((file) => relativePath(outputDir, joinPath(destDir, file))) pathMap.set(sourcePath, relFiles) filesCopied += copied.length @@ -85,7 +81,7 @@ export async function copyConfigKeyEntry( const uniqueDestPath = await findUniqueDestPath(destDir, basename(fullPath)) await copyFile(fullPath, uniqueDestPath) const outputRelative = relativePath(outputDir, uniqueDestPath) - options.stdout.write(`Copied '${sourcePath}' to ${outputRelative}\n`) + options.stdout.write(`Included '${sourcePath}'\n`) pathMap.set(sourcePath, outputRelative) filesCopied += 1 } diff --git a/packages/app/src/cli/services/build/steps/include-assets/copy-source-entry.test.ts b/packages/app/src/cli/services/build/steps/include-assets/copy-source-entry.test.ts index b87809ab807..1a98fdb70a8 100644 --- a/packages/app/src/cli/services/build/steps/include-assets/copy-source-entry.test.ts +++ b/packages/app/src/cli/services/build/steps/include-assets/copy-source-entry.test.ts @@ -51,7 +51,7 @@ describe('copySourceEntry', () => { expect(fs.copyFile).toHaveBeenCalledWith('/ext/src/icon.png', '/out/assets/icon.png') expect(result.filesCopied).toBe(1) expect(result.outputPaths).toEqual(['assets/icon.png']) - expect(mockStdout.write).toHaveBeenCalledWith('Copied src/icon.png to assets/icon.png\n') + expect(mockStdout.write).toHaveBeenCalledWith('Included src/icon.png\n') }) test('copies directory under its own name when no destination is given', async () => { @@ -71,27 +71,7 @@ describe('copySourceEntry', () => { expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/ext/dist', '/out/dist') expect(result.filesCopied).toBe(2) expect(result.outputPaths).toEqual(['dist/index.html', 'dist/logo.png']) - expect(mockStdout.write).toHaveBeenCalledWith('Copied dist to dist\n') - }) - - test('merges directory contents into output root when no destination and preserveStructure is false', async () => { - // Given - vi.mocked(fs.fileExists).mockResolvedValue(true) - vi.mocked(fs.isDirectory).mockResolvedValue(true) - vi.mocked(fs.copyDirectoryContents).mockResolvedValue() - vi.mocked(fs.glob).mockResolvedValue(['a.js', 'b.js', 'c.js']) - - // When - const result = await copySourceEntry( - {source: 'build', destination: undefined, baseDir: '/ext', outputDir: '/out', preserveStructure: false}, - {stdout: mockStdout}, - ) - - // Then - expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/ext/build', '/out') - expect(result.filesCopied).toBe(3) - expect(result.outputPaths).toEqual(['a.js', 'b.js', 'c.js']) - expect(mockStdout.write).toHaveBeenCalledWith('Copied contents of build to output root\n') + expect(mockStdout.write).toHaveBeenCalledWith('Included dist\n') }) test('copies file to basename in outputDir when source is a file and no destination given', async () => { @@ -111,7 +91,7 @@ describe('copySourceEntry', () => { expect(fs.copyFile).toHaveBeenCalledWith('/ext/README.md', '/out/README.md') expect(result.filesCopied).toBe(1) expect(result.outputPaths).toEqual(['README.md']) - expect(mockStdout.write).toHaveBeenCalledWith('Copied README.md to README.md\n') + expect(mockStdout.write).toHaveBeenCalledWith('Included README.md\n') }) test('copies directory to explicit destination path', async () => { @@ -131,7 +111,7 @@ describe('copySourceEntry', () => { expect(fs.copyDirectoryContents).toHaveBeenCalledWith('/ext/dist', '/out/vendor/dist') expect(result.filesCopied).toBe(1) expect(result.outputPaths).toEqual(['vendor/dist/x.js']) - expect(mockStdout.write).toHaveBeenCalledWith('Copied dist to vendor/dist\n') + expect(mockStdout.write).toHaveBeenCalledWith('Included dist\n') }) test('returns count of files discovered in destination directory after directory copy', async () => { @@ -150,7 +130,7 @@ describe('copySourceEntry', () => { // Then — count comes from glob on destPath, not a constant expect(result.filesCopied).toBe(5) - expect(result.outputPaths).toEqual(['a.js', 'b.js', 'c.js', 'd.js', 'e.js']) + expect(result.outputPaths).toEqual(['theme/a.js', 'theme/b.js', 'theme/c.js', 'theme/d.js', 'theme/e.js']) }) test('creates parent directories before copying a file', async () => { diff --git a/packages/app/src/cli/services/build/steps/include-assets/copy-source-entry.ts b/packages/app/src/cli/services/build/steps/include-assets/copy-source-entry.ts index 6499ba32da2..9f153c73415 100644 --- a/packages/app/src/cli/services/build/steps/include-assets/copy-source-entry.ts +++ b/packages/app/src/cli/services/build/steps/include-assets/copy-source-entry.ts @@ -16,7 +16,7 @@ export async function copySourceEntry( }, options: {stdout: NodeJS.WritableStream}, ): Promise<{filesCopied: number; outputPaths: string[]}> { - const {source, destination, baseDir, outputDir, preserveStructure} = config + const {source, destination, baseDir, outputDir} = config const sourcePath = joinPath(baseDir, source) if (!(await fileExists(sourcePath))) { throw new Error(`Source does not exist: ${sourcePath}`)