diff --git a/packages/cli-command/src/smartsnap.js b/packages/cli-command/src/smartsnap.js index f815b17aa..a3567106d 100644 --- a/packages/cli-command/src/smartsnap.js +++ b/packages/cli-command/src/smartsnap.js @@ -107,6 +107,58 @@ function gitProjectRoot() { return git(['rev-parse', '--show-toplevel']).trim(); } +// Parse `git diff --unified=0` hunk headers to find which line ranges are new +// or changed (on the HEAD side) for each file, then key them by the file's +// index in the stats `files` array — the same index module/source refs use, so +// the graph can join them without a path lookup. `--unified=0` drops context +// lines so every hunk header bounds an actual change; `--no-renames` makes a +// rename surface as add+delete so the new path is always concrete. +// +// Files in the diff that aren't tracked in `files` (e.g. node_modules, +// .storybook, or anything the stats compactor didn't index) are skipped — the +// graph can only reason about indexed files. Returns { : [[start, end], ...] }. +export function getAffectedFileLocations(baseRef, files) { + assertSafeRef(baseRef); + const diff = git(['diff', '--unified=0', '--no-color', '--no-renames', baseRef, 'HEAD', '--']); + + // The `files` array uses the platform separator (path.relative → back-slash + // on Windows); git diff always emits forward slashes. Normalize both to + // forward-slash for the path→index lookup. + const toPosix = p => p.split(path.sep).join('/'); + const indexByPath = new Map(files.map((f, i) => [toPosix(f), i])); + + // @@ -a,b +c,d @@ — the new-side `+c,d` is what we want. `d` defaults to 1 + // when omitted; `d === 0` is a pure deletion anchored at line c (no added + // lines), so it contributes no range. + const HUNK = /^@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@/; + + const locations = {}; + let currentIdx; + for (const line of diff.split('\n')) { + // `+++ b/` opens a file's hunks with its new-side path; a pure + // deletion shows `+++ /dev/null`, so leave currentIdx unset and skip it. + if (line.startsWith('+++ ')) { + const p = line.slice(4); + if (p === '/dev/null') { currentIdx = undefined; continue; } + /* istanbul ignore next: git always prefixes the new-side path with `b/` + (anything else is `/dev/null`, handled above), so the bare `: p` + fallback is defensive and unreachable in real diff output */ + const rel = p.startsWith('b/') ? p.slice(2) : p; + currentIdx = indexByPath.get(rel); + continue; + } + if (currentIdx === undefined) continue; + const m = HUNK.exec(line); + if (!m) continue; + const start = parseInt(m[1], 10); + const count = m[2] === undefined ? 1 : parseInt(m[2], 10); + if (count === 0) continue; + if (!locations[currentIdx]) locations[currentIdx] = []; + locations[currentIdx].push([start, start + count - 1]); + } + return locations; +} + // Paths under these directories are dependencies / framework wiring rather // than first-party source, so we don't track them in the file index. const EXCLUDED_DIRS = new Set(['node_modules', '.storybook']); @@ -147,6 +199,11 @@ function transformModule(m, fileIndex, projectRoot) { if (copy.type === 'src' && typeof copy.source === 'string') { copy.source = resolveAndIndex(copy.source, fileIndex, projectRoot); } + // `loc` (when present) is an array of `{ start, end }` source spans; the + // graph expects each span as a `[start, end]` tuple instead. + if (Array.isArray(copy.loc)) { + copy.loc = copy.loc.map(l => [l.start, l.end]); + } return copy; }; @@ -216,7 +273,7 @@ async function pollGraphStatus(percy, buildId, log) { const res = await percy.client.getStatus('smartsnap_graph', [buildId]); const status = res?.status; log.debug(`SmartSnap: graph status (attempt ${i + 1}) = ${status}`); - if (status === 'done' || status === 'failure') return { status, data: res?.data }; + if (status === 'done' || status === 'failed') return { status, data: res?.data }; if (i < POLL_ATTEMPTS - 1) await new Promise(r => setTimeout(r, POLL_INTERVAL_MS)); } return { status: null }; @@ -430,10 +487,12 @@ export function extractStorybookPaths(snapshots, normalizeImportPath, log) { // response body that used to come from `getSmartsnapGraphData`. Bails to a // full snapshot set if the job doesn't reach `done`. export async function runGraphGeneration(percy, buildId, payload, log) { - const { files, modules, storybookPaths, affectedNodes } = payload; - log.debug(`SmartSnap: starting graph generation job ${JSON.stringify({ buildId, files, modules, storybookPaths, affectedNodes })}`); + const { files, modules, storybookPaths, affectedNodes, affectedFileLocations } = payload; + log.debug(`SmartSnap: starting graph generation job ${JSON.stringify({ buildId, files, modules, storybookPaths, affectedNodes, affectedFileLocations })}`); + // Pass camelCase to the client, which snake_cases it to `affected_file_locations` + // for the API (same convention as storybookPaths → storybook_paths). await percy.client.generateSmartsnapGraph(buildId, { - files, modules, storybookPaths, affectedNodes + files, modules, storybookPaths, affectedNodes, affectedFileLocations }); const { status, data } = await pollGraphStatus(percy, buildId, log); @@ -583,7 +642,10 @@ export async function applySmartSnap(percy, snapshots, smartSnapConfig, buildDir affectedNodes = [...affectedNodes, ...packageAffectedNodes]; } - const data = await runGraphGeneration(percy, buildId, { files, modules, storybookPaths, affectedNodes }, log); + // Line-level diff ranges for the indexed files, keyed by their `files` index. + const affectedFileLocations = getAffectedFileLocations(baseRef, files); + + const data = await runGraphGeneration(percy, buildId, { files, modules, storybookPaths, affectedNodes, affectedFileLocations }, log); maybeWriteTrace(trace, data, log); diff --git a/packages/cli-command/test/smartsnap.test.js b/packages/cli-command/test/smartsnap.test.js index 53c9a3be8..f9a4a0864 100644 --- a/packages/cli-command/test/smartsnap.test.js +++ b/packages/cli-command/test/smartsnap.test.js @@ -11,6 +11,7 @@ import { assertNoBailOnChanges, enforceUntraced, getAffectedPackages, + getAffectedFileLocations, extractStorybookPaths, runGraphGeneration, maybeWriteTrace, @@ -148,12 +149,12 @@ describe('smartsnap', () => { { id: '/root/src/A.js', imports: [ - { type: 'src', source: '/root/src/B.js' }, + { type: 'src', source: '/root/src/B.js', loc: [{ start: 38, end: 38 }, { start: 40, end: 42 }] }, { type: 'src', source: '/root/src/B.js' }, // duplicate → reuses the existing index { type: 'src', source: 'lib/rel.js' }, // non-absolute → returned as-is, not indexed { type: 'module', source: 'react' } ], - passThroughExports: [{ type: 'src', source: '/root/src/C.js' }], + passThroughExports: [{ type: 'src', source: '/root/src/C.js', loc: [{ start: 5, end: 5 }] }], nonPassThroughExports: [{ type: 'module', source: 'lodash' }] }, { id: '/root/node_modules/dep/index.js' }, // excluded → string id → dropped @@ -174,7 +175,9 @@ describe('smartsnap', () => { expect(res.modules[0].imports[1].source).toEqual(1); // duplicate → same index expect(res.modules[0].imports[2].source).toEqual('lib/rel.js'); // non-absolute → as-is expect(res.modules[0].imports[3].source).toEqual('react'); // module ref → untouched + expect(res.modules[0].imports[0].loc).toEqual([[38, 38], [40, 42]]); // {start,end} spans → [start,end] tuples expect(res.modules[0].passThroughExports[0].source).toEqual(2); + expect(res.modules[0].passThroughExports[0].loc).toEqual([[5, 5]]); expect(res.modules[0].nonPassThroughExports).toEqual([{ type: 'module', source: 'lodash' }]); expect(res.modules[1]).toEqual({}); }); @@ -363,7 +366,14 @@ describe('smartsnap', () => { getStatus: async () => ({ status: 'done', data }) } }; - let payload = { files: ['f'], modules: [{ id: 0 }], storybookPaths: ['p'], affectedNodes: ['a'] }; + // affectedFileLocations is forwarded verbatim; the client snake_cases it. + let payload = { + files: ['f'], + modules: [{ id: 0 }], + storybookPaths: ['p'], + affectedNodes: ['a'], + affectedFileLocations: { 0: [[3, 3], [6, 7]] } + }; let res = await runGraphGeneration(percy, 'bld-1', payload, log); @@ -376,7 +386,7 @@ describe('smartsnap', () => { let percy = { client: { generateSmartsnapGraph: async () => {}, - getStatus: async () => ({ status: 'failure' }) + getStatus: async () => ({ status: 'failed' }) } }; await expectBail( @@ -609,6 +619,65 @@ describe('smartsnap', () => { }); }); + describe('getAffectedFileLocations()', () => { + let origCwd = process.cwd(); + let repos = []; + afterEach(() => { + process.chdir(origCwd); + for (let d of repos.splice(0)) fs.rmSync(d, { recursive: true, force: true }); + }); + + function setup(seed, changed) { + let info = makeRepo(seed, changed); + repos.push(info.dir); + process.chdir(info.dir); + return info; + } + + it('maps changed line ranges to file index, skipping unindexed and deleted files', () => { + let { dir, baseSha } = setup( + { + 'src/A.js': 'a\nb\nc\nd\ne\n', // indexed; line 3 changed + lines 6-7 appended + 'src/del.js': 'x\n' // indexed; deleted at HEAD → no new-side lines + }, + { + 'src/A.js': 'a\nb\nC\nd\ne\nf\ng\n', + 'src/B.js': 'new\n' // NOT in the files index → skipped + }); + // makeRepo's writeAll can't delete; drop del.js in a follow-up commit so the + // baseSha→HEAD diff carries its `+++ /dev/null` deletion hunk. + fs.rmSync(path.join(dir, 'src/del.js')); + git(['add', '-A'], dir); + git(['commit', '-qm', 'remove del'], dir); + + // files index: A=0, del=1; B.js is absent (not indexed). + let res = getAffectedFileLocations(baseSha, ['src/A.js', 'src/del.js']); + + // A.js: `@@ -3 +3 @@` → [3,3] and `@@ -5,0 +6,2 @@` → [6,7]. + // del.js shows `+++ /dev/null` (pure deletion) → no entry. B.js unindexed → no entry. + expect(res).toEqual({ 0: [[3, 3], [6, 7]] }); + }); + + it('ignores pure-deletion hunks that add no new lines', () => { + let { baseSha } = setup( + { 'src/A.js': '1\n2\n3\n' }, // line 2 removed, file kept + { 'src/A.js': '1\n3\n' }); + // `@@ -2 +1,0 @@` → new-side count 0 → no range contributed. + expect(getAffectedFileLocations(baseSha, ['src/A.js'])).toEqual({}); + }); + + it('returns an empty map when there is no diff', () => { + let { baseSha } = setup({ 'src/A.js': 'a\n' }); + // baseSha === HEAD (no second commit) → `git diff HEAD` is empty. + expect(getAffectedFileLocations(baseSha, ['src/A.js'])).toEqual({}); + }); + + it('bails on an unsafe base ref before shelling out to git', () => { + expect(() => getAffectedFileLocations('--upload-pack=evil', [])) + .toThrowMatching(e => e instanceof SmartSnapBailError && e.message.includes('unsafe baseline ref')); + }); + }); + describe('applySmartSnap() [integration]', () => { let origCwd = process.cwd(); let repos = []; diff --git a/packages/client/src/client.js b/packages/client/src/client.js index 43b21614d..45bc3d647 100644 --- a/packages/client/src/client.js +++ b/packages/client/src/client.js @@ -457,7 +457,8 @@ export class PercyClient { files, modules, storybookPaths, - affectedNodes + affectedNodes, + affectedFileLocations } = {}) { this.log.debug(`Smartsnap: enqueueing graph build for build ${buildId}...`); return this.post('smartsnap/generate-graph', { @@ -465,7 +466,8 @@ export class PercyClient { files, modules, storybook_paths: storybookPaths, - affected_nodes: affectedNodes + affected_nodes: affectedNodes, + affected_file_locations: affectedFileLocations }, { identifier: 'smartsnap.generate_graph' }); } diff --git a/packages/client/test/client.test.js b/packages/client/test/client.test.js index 84f0e3b0a..c17d8b406 100644 --- a/packages/client/test/client.test.js +++ b/packages/client/test/client.test.js @@ -901,7 +901,8 @@ describe('PercyClient', () => { files: ['a.js', 'b.js'], modules: [{ id: 1, name: 'mod' }], storybookPaths: ['stories/a.js'], - affectedNodes: ['node-1'] + affectedNodes: ['node-1'], + affectedFileLocations: { 0: [[3, 3], [6, 7]], 1: [[1, 1]] } })).toBeResolvedTo({ status: 'queued' }); expect(api.requests['/smartsnap/generate-graph']).toBeDefined(); @@ -911,7 +912,8 @@ describe('PercyClient', () => { files: ['a.js', 'b.js'], modules: [{ id: 1, name: 'mod' }], storybook_paths: ['stories/a.js'], - affected_nodes: ['node-1'] + affected_nodes: ['node-1'], + affected_file_locations: { 0: [[3, 3], [6, 7]], 1: [[1, 1]] } }); });