From 69ede53395e0893eea0bfb223d1c009912781347 Mon Sep 17 00:00:00 2001 From: RaghavsBrowserStack Date: Tue, 23 Jun 2026 10:43:50 +0530 Subject: [PATCH 1/3] Revert "PPLT-5495: Add file diff data to generate graph (#2273)" This reverts commit 2392017274a64f81ec019cc6306dbc1082e25a50. --- packages/cli-command/src/smartsnap.js | 72 ++----------------- packages/cli-command/test/smartsnap.test.js | 77 ++------------------- packages/client/src/client.js | 6 +- packages/client/test/client.test.js | 6 +- 4 files changed, 13 insertions(+), 148 deletions(-) diff --git a/packages/cli-command/src/smartsnap.js b/packages/cli-command/src/smartsnap.js index a3567106d..f815b17aa 100644 --- a/packages/cli-command/src/smartsnap.js +++ b/packages/cli-command/src/smartsnap.js @@ -107,58 +107,6 @@ 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']); @@ -199,11 +147,6 @@ 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; }; @@ -273,7 +216,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 === 'failed') return { status, data: res?.data }; + if (status === 'done' || status === 'failure') return { status, data: res?.data }; if (i < POLL_ATTEMPTS - 1) await new Promise(r => setTimeout(r, POLL_INTERVAL_MS)); } return { status: null }; @@ -487,12 +430,10 @@ 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, 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). + 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, affectedFileLocations + files, modules, storybookPaths, affectedNodes }); const { status, data } = await pollGraphStatus(percy, buildId, log); @@ -642,10 +583,7 @@ export async function applySmartSnap(percy, snapshots, smartSnapConfig, buildDir affectedNodes = [...affectedNodes, ...packageAffectedNodes]; } - // 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); + const data = await runGraphGeneration(percy, buildId, { files, modules, storybookPaths, affectedNodes }, log); maybeWriteTrace(trace, data, log); diff --git a/packages/cli-command/test/smartsnap.test.js b/packages/cli-command/test/smartsnap.test.js index f9a4a0864..53c9a3be8 100644 --- a/packages/cli-command/test/smartsnap.test.js +++ b/packages/cli-command/test/smartsnap.test.js @@ -11,7 +11,6 @@ import { assertNoBailOnChanges, enforceUntraced, getAffectedPackages, - getAffectedFileLocations, extractStorybookPaths, runGraphGeneration, maybeWriteTrace, @@ -149,12 +148,12 @@ describe('smartsnap', () => { { id: '/root/src/A.js', imports: [ - { type: 'src', source: '/root/src/B.js', loc: [{ start: 38, end: 38 }, { start: 40, end: 42 }] }, + { type: 'src', source: '/root/src/B.js' }, { 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', loc: [{ start: 5, end: 5 }] }], + passThroughExports: [{ type: 'src', source: '/root/src/C.js' }], nonPassThroughExports: [{ type: 'module', source: 'lodash' }] }, { id: '/root/node_modules/dep/index.js' }, // excluded → string id → dropped @@ -175,9 +174,7 @@ 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({}); }); @@ -366,14 +363,7 @@ describe('smartsnap', () => { getStatus: async () => ({ status: 'done', data }) } }; - // 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 payload = { files: ['f'], modules: [{ id: 0 }], storybookPaths: ['p'], affectedNodes: ['a'] }; let res = await runGraphGeneration(percy, 'bld-1', payload, log); @@ -386,7 +376,7 @@ describe('smartsnap', () => { let percy = { client: { generateSmartsnapGraph: async () => {}, - getStatus: async () => ({ status: 'failed' }) + getStatus: async () => ({ status: 'failure' }) } }; await expectBail( @@ -619,65 +609,6 @@ 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 df2b944f2..c3cff616b 100644 --- a/packages/client/src/client.js +++ b/packages/client/src/client.js @@ -457,8 +457,7 @@ export class PercyClient { files, modules, storybookPaths, - affectedNodes, - affectedFileLocations + affectedNodes } = {}) { this.log.debug(`Smartsnap: enqueueing graph build for build ${buildId}...`); return this.post('smartsnap/generate-graph', { @@ -466,8 +465,7 @@ export class PercyClient { files, modules, storybook_paths: storybookPaths, - affected_nodes: affectedNodes, - affected_file_locations: affectedFileLocations + affected_nodes: affectedNodes }, { identifier: 'smartsnap.generate_graph' }); } diff --git a/packages/client/test/client.test.js b/packages/client/test/client.test.js index ffa81e503..597a58b9e 100644 --- a/packages/client/test/client.test.js +++ b/packages/client/test/client.test.js @@ -901,8 +901,7 @@ describe('PercyClient', () => { files: ['a.js', 'b.js'], modules: [{ id: 1, name: 'mod' }], storybookPaths: ['stories/a.js'], - affectedNodes: ['node-1'], - affectedFileLocations: { 0: [[3, 3], [6, 7]], 1: [[1, 1]] } + affectedNodes: ['node-1'] })).toBeResolvedTo({ status: 'queued' }); expect(api.requests['/smartsnap/generate-graph']).toBeDefined(); @@ -912,8 +911,7 @@ describe('PercyClient', () => { files: ['a.js', 'b.js'], modules: [{ id: 1, name: 'mod' }], storybook_paths: ['stories/a.js'], - affected_nodes: ['node-1'], - affected_file_locations: { 0: [[3, 3], [6, 7]], 1: [[1, 1]] } + affected_nodes: ['node-1'] }); }); From 1cb6927d772c7a3f5948aace71940bab01a0e8b4 Mon Sep 17 00:00:00 2001 From: RaghavsBrowserStack Date: Tue, 23 Jun 2026 10:44:47 +0530 Subject: [PATCH 2/3] Revert "Export applySmartSnap functionality from CLI (#2207)" This reverts commit d3f7f71ea4bb31c31b66df8daf7219c8a2c86f0f. --- .github/workflows/Semgrep.yml | 8 +- packages/cli-command/package.json | 8 +- packages/cli-command/src/graphTrace.js | 154 ---- .../cli-command/src/graphTraceTemplate.html | 349 --------- packages/cli-command/src/index.js | 1 - packages/cli-command/src/lockfileDiff.js | 166 ----- packages/cli-command/src/smartsnap.js | 591 --------------- packages/cli-command/test/graphTrace.test.js | 247 ------- packages/cli-command/test/index.test.js | 17 - .../cli-command/test/lockfileDiff.test.js | 115 --- packages/cli-command/test/smartsnap.test.js | 674 ------------------ packages/client/src/client.js | 52 +- packages/client/test/client.test.js | 119 ---- yarn.lock | 647 +---------------- 14 files changed, 9 insertions(+), 3139 deletions(-) delete mode 100644 packages/cli-command/src/graphTrace.js delete mode 100644 packages/cli-command/src/graphTraceTemplate.html delete mode 100644 packages/cli-command/src/lockfileDiff.js delete mode 100644 packages/cli-command/src/smartsnap.js delete mode 100644 packages/cli-command/test/graphTrace.test.js delete mode 100644 packages/cli-command/test/index.test.js delete mode 100644 packages/cli-command/test/lockfileDiff.test.js delete mode 100644 packages/cli-command/test/smartsnap.test.js diff --git a/.github/workflows/Semgrep.yml b/.github/workflows/Semgrep.yml index b08eda5ac..d2614fa07 100644 --- a/.github/workflows/Semgrep.yml +++ b/.github/workflows/Semgrep.yml @@ -26,12 +26,8 @@ jobs: runs-on: ubuntu-latest container: - # A Docker image with Semgrep installed. - # Pinned to >=1.164.0: the old `returntocorp/semgrep:latest` (semgrep 1.162.0) - # has a bug where `semgrep ci --sarif` counts nosemgrep-suppressed findings as - # blocking and exits non-zero (fixed in 1.164.0), so inline `// nosemgrep` - # suppressions were ignored. See https://semgrep.dev/docs CHANGELOG v1.164.0. - image: semgrep/semgrep:1.164.0 + # A Docker image with Semgrep installed. Do not change this. + image: returntocorp/semgrep # Skip any PR created by dependabot to avoid permission issues: if: (github.actor != 'dependabot[bot]') diff --git a/packages/cli-command/package.json b/packages/cli-command/package.json index cc94ef17f..7d69e694e 100644 --- a/packages/cli-command/package.json +++ b/packages/cli-command/package.json @@ -27,7 +27,6 @@ ".": "./dist/index.js", "./flags": "./dist/flags.js", "./utils": "./dist/utils.js", - "./smartsnap": "./dist/smartsnap.js", "./test/helpers": "./test/helpers.js" }, "scripts": { @@ -39,11 +38,6 @@ "dependencies": { "@percy/config": "1.32.2", "@percy/core": "1.32.2", - "@percy/logger": "1.32.2", - "glob-to-regexp": "^0.4.1", - "stream-json": "^1.8.0" - }, - "optionalDependencies": { - "snyk-nodejs-lockfile-parser": "2.7.1" + "@percy/logger": "1.32.2" } } diff --git a/packages/cli-command/src/graphTrace.js b/packages/cli-command/src/graphTrace.js deleted file mode 100644 index f869286c8..000000000 --- a/packages/cli-command/src/graphTrace.js +++ /dev/null @@ -1,154 +0,0 @@ -import fs from 'fs'; -import path from 'path'; -import url from 'url'; - -// Template resolution mirrors core/utils.js's secretPatterns.yml lookup: -// resolves relative to this file's URL so it works under src/ (dev) and -// dist/ (installed) without bundler help. The .html file is copied alongside -// by babel's copyFiles when cli-command is built. -const TEMPLATE_PATH = path.resolve(url.fileURLToPath(import.meta.url), '../graphTraceTemplate.html'); - -// Maps a (raw kind, changed) pair to the kind value the template expects: -// 'package' | 'component' | 'story' | 'is_relevant'. `changed: true` wins -// over the underlying kind so any node touched in the diff renders purple. -function templateKindOf(v) { - if (v.changed) return 'is_relevant'; - switch (v.kind) { - case 'dependency': return 'package'; - case 'component': return 'component'; - case 'story': return 'story'; - default: return 'component'; - } -} - -// Sort order within a column: packages left, components middle, stories right. -// `is_relevant` shares rank with components so a changed node doesn't jump -// out of its own group — it just recolors. -const KIND_RANK = { package: 0, component: 1, is_relevant: 1, story: 2 }; - -// Layout algorithm (ported from the original Ruby renderer): -// 1. col = longest-path depth reaching the vertex (read from the -// transitive-closure triples the API sends), with dependencies pinned -// to col 0. -// 2. Propagate over edges so col[target] > col[source]. Bounded loop -// guards against degenerate inputs. -// 3. Stories pushed past the rightmost non-story column. -// 4. Within each column, sort by (kind-rank, name) and assign row. -function computeLayout(rawVertices, edges, transitiveClosure) { - const n = rawVertices.length; - const vertices = rawVertices.map((v, i) => ({ - index: i, - name: v.file_path, - kind: v.kind, - changed: !!v.changed, - row: 0, - col: 0 - })); - - // 1. Seed col from incoming transitive-closure lengths. - const incomingMax = new Array(n).fill(0); - for (const triple of transitiveClosure) { - const [u, v, val] = triple; - if (u === v || val <= 0) continue; - if (v < 0 || v >= n) continue; - if (val > incomingMax[v]) incomingMax[v] = val; - } - for (let i = 0; i < n; i++) { - vertices[i].col = vertices[i].kind === 'dependency' ? 0 : incomingMax[i] + 1; - } - - // 2. Propagate edge constraint. n+2 iterations is enough for any DAG - // and bounds the work on accidentally-cyclic input. - const iterations = n + 2; - for (let iter = 0; iter < iterations; iter++) { - let changed = false; - for (const [s, t] of edges) { - if (s < 0 || s >= n || t < 0 || t >= n) continue; - if (vertices[s].col < vertices[t].col) continue; - vertices[t].col = vertices[s].col + 1; - changed = true; - } - if (!changed) break; - } - - // 3. Stories rightmost. Two passes: max across non-stories first, then - // push every story past that boundary. Folding into one loop would let - // stories visited before the last non-story keep a stale max. - let furthestNonStory = 0; - for (const v of vertices) { - if (v.kind === 'story') continue; - if (v.col > furthestNonStory) furthestNonStory = v.col; - } - for (const v of vertices) { - if (v.kind !== 'story') continue; - if (v.col < furthestNonStory + 1) v.col = furthestNonStory + 1; - } - - // 4. Group by column, sort by (kind-rank, name), assign row. - const groups = new Map(); - for (const v of vertices) { - let list = groups.get(v.col); - if (!list) groups.set(v.col, list = []); - list.push(v); - } - const rankOf = v => { - const r = KIND_RANK[templateKindOf(v)]; - /* istanbul ignore next: templateKindOf always returns a kind present in - KIND_RANK, so the `=== undefined` fallback is defensive */ - return r === undefined ? 99 : r; - }; - for (const list of groups.values()) { - list.sort((a, b) => { - const ra = rankOf(a); - const rb = rankOf(b); - if (ra !== rb) return ra - rb; - // Byte-wise compare on name to match Ruby's String#<=> behaviour. - if (a.name < b.name) return -1; - if (a.name > b.name) return 1; - return 0; - }); - list.forEach((v, row) => { v.row = row; }); - } - - // 5. Final shape the template consumes: drop `changed`, fold it into kind. - return vertices.map(v => ({ - index: v.index, - name: v.name, - row: v.row, - col: v.col, - kind: templateKindOf(v) - })); -} - -// Escapes characters that have meaning inside a `; `` cover HTML comment confusion; U+2028 -// and U+2029 are valid JSON but illegal in JS string literals pre-ES2019 and -// have historically been XSS sinks. -const LS = String.fromCharCode(0x2028); -const PS = String.fromCharCode(0x2029); -function safeJson(obj) { - return JSON.stringify(obj) - .replace(/<\//g, '<\\/') - .replace(/`; - - function hostileLine() { - return embeddedJson(renderGraphTraceHtml({ - vertices: [{ kind: 'component', file_path: hostile }], - edges: [], - transitiveClosureMatrixSparse: [] - }), 'vertices'); - } - - it('escapes " { - let line = hostileLine(); - expect(line).not.toContain(''); - expect(line).toContain('<\\/script>'); - }); - - it('escapes HTML comment open and close markers', () => { - let line = hostileLine(); - expect(line).toContain('<\\!--'); - expect(line).toContain('--\\>'); - }); - - it('escapes U+2028 and U+2029 line/paragraph separators', () => { - let line = hostileLine(); - expect(line).not.toContain(LS); - expect(line).not.toContain(PS); - expect(line).toContain('\\u2028'); - expect(line).toContain('\\u2029'); - }); - - it('escapes only the dangerous sequences, leaving the payload intact', () => { - // The output is embedded in a