From 51ab2ba1664edc2cc50ad859f0bb1e7e1c36b351 Mon Sep 17 00:00:00 2001 From: RaghavsBrowserStack Date: Tue, 9 Jun 2026 21:58:15 +0530 Subject: [PATCH 1/5] add file diff handling --- packages/cli-command/src/smartsnap.js | 72 +++++++++++++++++-- packages/cli-command/test/smartsnap.test.js | 79 ++++++++++++++++++++- 2 files changed, 143 insertions(+), 8 deletions(-) diff --git a/packages/cli-command/src/smartsnap.js b/packages/cli-command/src/smartsnap.js index f815b17aa..d759d2433 100644 --- a/packages/cli-command/src/smartsnap.js +++ b/packages/cli-command/src/smartsnap.js @@ -107,6 +107,55 @@ 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; } + 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 +196,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; }; @@ -430,11 +484,14 @@ 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 })}`); - await percy.client.generateSmartsnapGraph(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 })}`); + const graphPayload = { files, modules, storybookPaths, affectedNodes }; + // Attach only when the caller supplied it (an empty `{}` from the no-diff path + // is still a value and is sent); callers that omit it entirely don't send the + // key. The API maps file index → changed [start, end] line ranges. + if (affectedFileLocations) graphPayload.affected_file_locations = affectedFileLocations; + await percy.client.generateSmartsnapGraph(buildId, graphPayload); const { status, data } = await pollGraphStatus(percy, buildId, log); if (status !== 'done') { @@ -583,7 +640,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..608d5d3c9 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({}); }); @@ -371,6 +374,27 @@ describe('smartsnap', () => { expect(generate).toHaveBeenCalledWith('bld-1', payload); }); + it('forwards affectedFileLocations to the API as affected_file_locations', async () => { + let log = mockLog(); + let generate = jasmine.createSpy('generateSmartsnapGraph'); + let percy = { + client: { + generateSmartsnapGraph: generate, + getStatus: async () => ({ status: 'done', data: { affected_stories: [] } }) + } + }; + let affectedFileLocations = { 0: [[3, 3], [6, 7]], 2: [[1, 1]] }; + let payload = { files: ['f'], modules: [], storybookPaths: [], affectedNodes: ['a'], affectedFileLocations }; + + await runGraphGeneration(percy, 'bld-1', payload, log); + + // affectedFileLocations is renamed to the snake_case key the API expects; + // the camelCase key is not forwarded. + expect(generate).toHaveBeenCalledWith('bld-1', { + files: ['f'], modules: [], storybookPaths: [], affectedNodes: ['a'], affected_file_locations: affectedFileLocations + }); + }); + it('bails when the job does not reach done', async () => { let log = mockLog(); let percy = { @@ -609,6 +633,57 @@ 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('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 = []; From a959c3bbbd4eb51706a4eee82e8ae4648546998f Mon Sep 17 00:00:00 2001 From: RaghavsBrowserStack Date: Tue, 9 Jun 2026 22:07:30 +0530 Subject: [PATCH 2/5] improve coverage --- packages/cli-command/src/smartsnap.js | 3 +++ packages/cli-command/test/smartsnap.test.js | 8 ++++++++ 2 files changed, 11 insertions(+) diff --git a/packages/cli-command/src/smartsnap.js b/packages/cli-command/src/smartsnap.js index d759d2433..1c8f93057 100644 --- a/packages/cli-command/src/smartsnap.js +++ b/packages/cli-command/src/smartsnap.js @@ -140,6 +140,9 @@ export function getAffectedFileLocations(baseRef, files) { 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; diff --git a/packages/cli-command/test/smartsnap.test.js b/packages/cli-command/test/smartsnap.test.js index 608d5d3c9..27d701415 100644 --- a/packages/cli-command/test/smartsnap.test.js +++ b/packages/cli-command/test/smartsnap.test.js @@ -672,6 +672,14 @@ describe('smartsnap', () => { 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. From db8d27b6465ab48bcad8769fae01440ae645848a Mon Sep 17 00:00:00 2001 From: RaghavsBrowserStack Date: Wed, 10 Jun 2026 11:25:35 +0530 Subject: [PATCH 3/5] fix some stuff --- packages/cli-command/src/smartsnap.js | 11 ++++----- packages/cli-command/test/smartsnap.test.js | 27 ++++----------------- packages/client/src/client.js | 6 +++-- packages/client/test/client.test.js | 6 +++-- 4 files changed, 18 insertions(+), 32 deletions(-) diff --git a/packages/cli-command/src/smartsnap.js b/packages/cli-command/src/smartsnap.js index 1c8f93057..9d9f8abc9 100644 --- a/packages/cli-command/src/smartsnap.js +++ b/packages/cli-command/src/smartsnap.js @@ -489,12 +489,11 @@ export function extractStorybookPaths(snapshots, normalizeImportPath, log) { export async function runGraphGeneration(percy, buildId, payload, log) { const { files, modules, storybookPaths, affectedNodes, affectedFileLocations } = payload; log.debug(`SmartSnap: starting graph generation job ${JSON.stringify({ buildId, files, modules, storybookPaths, affectedNodes, affectedFileLocations })}`); - const graphPayload = { files, modules, storybookPaths, affectedNodes }; - // Attach only when the caller supplied it (an empty `{}` from the no-diff path - // is still a value and is sent); callers that omit it entirely don't send the - // key. The API maps file index → changed [start, end] line ranges. - if (affectedFileLocations) graphPayload.affected_file_locations = affectedFileLocations; - await percy.client.generateSmartsnapGraph(buildId, graphPayload); + // 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, affectedFileLocations + }); const { status, data } = await pollGraphStatus(percy, buildId, log); if (status !== 'done') { diff --git a/packages/cli-command/test/smartsnap.test.js b/packages/cli-command/test/smartsnap.test.js index 27d701415..985b19dd7 100644 --- a/packages/cli-command/test/smartsnap.test.js +++ b/packages/cli-command/test/smartsnap.test.js @@ -366,7 +366,11 @@ 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); @@ -374,27 +378,6 @@ describe('smartsnap', () => { expect(generate).toHaveBeenCalledWith('bld-1', payload); }); - it('forwards affectedFileLocations to the API as affected_file_locations', async () => { - let log = mockLog(); - let generate = jasmine.createSpy('generateSmartsnapGraph'); - let percy = { - client: { - generateSmartsnapGraph: generate, - getStatus: async () => ({ status: 'done', data: { affected_stories: [] } }) - } - }; - let affectedFileLocations = { 0: [[3, 3], [6, 7]], 2: [[1, 1]] }; - let payload = { files: ['f'], modules: [], storybookPaths: [], affectedNodes: ['a'], affectedFileLocations }; - - await runGraphGeneration(percy, 'bld-1', payload, log); - - // affectedFileLocations is renamed to the snake_case key the API expects; - // the camelCase key is not forwarded. - expect(generate).toHaveBeenCalledWith('bld-1', { - files: ['f'], modules: [], storybookPaths: [], affectedNodes: ['a'], affected_file_locations: affectedFileLocations - }); - }); - it('bails when the job does not reach done', async () => { let log = mockLog(); let percy = { 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]] } }); }); From 6dba59bb5e23e71cb6e5c890fe4f87e31b973229 Mon Sep 17 00:00:00 2001 From: RaghavsBrowserStack Date: Wed, 10 Jun 2026 11:45:40 +0530 Subject: [PATCH 4/5] fix lint --- packages/cli-command/test/smartsnap.test.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/cli-command/test/smartsnap.test.js b/packages/cli-command/test/smartsnap.test.js index 985b19dd7..4ecd98fc2 100644 --- a/packages/cli-command/test/smartsnap.test.js +++ b/packages/cli-command/test/smartsnap.test.js @@ -368,7 +368,10 @@ describe('smartsnap', () => { }; // affectedFileLocations is forwarded verbatim; the client snake_cases it. let payload = { - files: ['f'], modules: [{ id: 0 }], storybookPaths: ['p'], affectedNodes: ['a'], + files: ['f'], + modules: [{ id: 0 }], + storybookPaths: ['p'], + affectedNodes: ['a'], affectedFileLocations: { 0: [[3, 3], [6, 7]] } }; From 59b0a665c5d75895b77375bc2985eb38ff9249de Mon Sep 17 00:00:00 2001 From: RaghavsBrowserStack Date: Wed, 10 Jun 2026 14:13:09 +0530 Subject: [PATCH 5/5] . --- packages/cli-command/src/smartsnap.js | 2 +- packages/cli-command/test/smartsnap.test.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/cli-command/src/smartsnap.js b/packages/cli-command/src/smartsnap.js index 9d9f8abc9..a3567106d 100644 --- a/packages/cli-command/src/smartsnap.js +++ b/packages/cli-command/src/smartsnap.js @@ -273,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 }; diff --git a/packages/cli-command/test/smartsnap.test.js b/packages/cli-command/test/smartsnap.test.js index 4ecd98fc2..f9a4a0864 100644 --- a/packages/cli-command/test/smartsnap.test.js +++ b/packages/cli-command/test/smartsnap.test.js @@ -386,7 +386,7 @@ describe('smartsnap', () => { let percy = { client: { generateSmartsnapGraph: async () => {}, - getStatus: async () => ({ status: 'failure' }) + getStatus: async () => ({ status: 'failed' }) } }; await expectBail(