Skip to content

Commit e279fdd

Browse files
Bill LeoutsakosBill Leoutsakos
authored andcommitted
fix(design-diff): trace collection writes without mixing unrelated properties
1 parent 309054c commit e279fdd

8 files changed

Lines changed: 149 additions & 12 deletions

File tree

scripts/design-diff/README.md

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,7 @@ scripts/design-diff/
9696
dependencies.ts Export-aware imports and source reference counts
9797
ast.ts Babel parsing and syntax normalization
9898
refactors.ts Supported literal/refactor normalization
99+
mutations.ts Referenced collection/property writes
99100
resolve.ts Bounded expression and import resolution
100101
inputs.ts Configured file-loaded documentation inputs
101102
infrastructure.ts Rendering lockfile dependency closure
@@ -179,7 +180,7 @@ The configured Fumadocs `OPENAPI_SPEC_FILES` list is parsed from Git in each rev
179180
JSON inputs are compared semantically, retaining array order and attributing changes to both
180181
the spec and configured renderer. Malformed/missing configured inputs flag with a limitation.
181182

182-
The graph reuses up to 10,000 import/export snapshots keyed by source blob, resolving their
183+
The graph reuses up to 32,768 import/export snapshots keyed by source blob, resolving their
183184
paths again for each revision. The resolver retains at most 32 parsed modules per revision,
184185
and requests Bun garbage collection between parser batches. These resource controls do
185186
not change evidence or decisions. The Node-based test runner uses its own garbage collector.

scripts/design-diff/dependencies.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -189,7 +189,7 @@ export class DependencyGraph {
189189
effects: effects.length ? fingerprint(effects) : undefined,
190190
}
191191
this.modules.set(file, module)
192-
if (moduleSnapshots.size >= 10000)
192+
if (moduleSnapshots.size >= 32768)
193193
moduleSnapshots.delete(moduleSnapshots.keys().next().value!)
194194
moduleSnapshots.set(cacheKey, { module, specifiers: [...specifiers] })
195195
} catch {

scripts/design-diff/extract/tsx.ts

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -287,8 +287,7 @@ export function extractTsx(resolver: Resolver, file: string): Definition[] {
287287
} else emit(path, 'review', 'native-options', evidence)
288288
},
289289
TaggedTemplateExpression(path) {
290-
// A tag is only a standalone visual definition for a known styling binding.
291-
// SQL and String.raw still participate when explicitly read by a visual input.
290+
/** A tag is only a standalone visual definition for a known styling binding. SQL and String.raw still participate when explicitly read by a visual input. */
292291
const tag = child(path, 'tag')
293292
const root = tag.isMemberExpression() ? child(tag, 'object') : tag
294293
if (!root.isIdentifier()) return

scripts/design-diff/mutations.ts

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
import type { Binding, NodePath } from '@babel/traverse'
2+
import * as t from '@babel/types'
3+
4+
/** Writes to a referenced collection remain relevant even when its binding is const. */
5+
export function mutations(binding: Binding, selected?: string): NodePath[] {
6+
const result = new Set<NodePath>()
7+
for (const reference of binding.referencePaths) {
8+
const first = reference.parentPath
9+
if (selected && first?.isMemberExpression() && first.node.object === reference.node) {
10+
const key = first.node.property
11+
const name =
12+
!first.node.computed && t.isIdentifier(key)
13+
? key.name
14+
: t.isStringLiteral(key)
15+
? key.value
16+
: undefined
17+
const method =
18+
first.parentPath.isCallExpression() && first.parentPath.node.callee === first.node
19+
if (name !== undefined && name !== selected && !method) continue
20+
}
21+
let value = reference
22+
let member = value.parentPath
23+
while (member?.isMemberExpression() && member.node.object === value.node) {
24+
const parent = member.parentPath
25+
if (
26+
(parent.isAssignmentExpression() && parent.node.left === member.node) ||
27+
parent.isUpdateExpression() ||
28+
parent.isUnaryExpression({ operator: 'delete' })
29+
)
30+
result.add(parent)
31+
const property = member.node.property
32+
const name = t.isIdentifier(property)
33+
? property.name
34+
: t.isStringLiteral(property)
35+
? property.value
36+
: ''
37+
if (
38+
parent.isCallExpression() &&
39+
parent.node.callee === member.node &&
40+
/^(?:push|pop|shift|unshift|splice|sort|reverse|fill|copyWithin|set|add|delete|clear)$/.test(
41+
name
42+
)
43+
)
44+
result.add(parent)
45+
value = member
46+
member = value.parentPath
47+
}
48+
}
49+
return [...result]
50+
}

scripts/design-diff/refactors.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import type traverse from '@babel/traverse'
22
import type { NodePath } from '@babel/traverse'
33
import * as t from '@babel/types'
4+
import { mutations } from '#design-diff/mutations'
45

56
/** Normalize the equivalent Object.entries record-map idiom without executing its callback. */
67
export function normalizeRefactors(ast: t.File, visit: typeof traverse): void {
@@ -97,7 +98,7 @@ export function normalizeLiteralAliases(ast: t.File, visit: typeof traverse): vo
9798
value = literal(path.get('expression') as NodePath, seen)
9899
else if (path.isReferencedIdentifier()) {
99100
const binding = path.scope.getBinding(path.node.name)
100-
if (binding?.constant && binding.path.isVariableDeclarator())
101+
if (binding?.constant && binding.path.isVariableDeclarator() && !mutations(binding).length)
101102
value = literal(binding.path.get('init') as NodePath, seen)
102103
} else if (path.isObjectExpression()) {
103104
const properties: t.ObjectProperty[] = []

scripts/design-diff/report.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,7 @@ export function serializeReport(report: Report, limit = REPORT_BYTES): string {
8585
}
8686
const json = () => `${JSON.stringify(copy, null, 2)}\n`
8787
let result = json()
88-
// Remove details first, then source groups. Stable prefixes make repeated output identical.
88+
/** Remove details first, then source groups. Stable prefixes make repeated output identical. */
8989
if (Buffer.byteLength(result) > limit) {
9090
for (const [i, finding] of copy.findings.entries()) {
9191
finding.changes = sample(finding.changes, `findings[${i}].changes.remaining`, 1)

scripts/design-diff/resolve.ts

Lines changed: 47 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import {
99
symbolName,
1010
traverse,
1111
} from '#design-diff/ast'
12+
import { mutations } from '#design-diff/mutations'
1213
import { previewValue } from '#design-diff/report'
1314
import type { SourceTree } from '#design-diff/source'
1415
import type { Data, Evidence } from '#design-diff/types'
@@ -160,8 +161,7 @@ export class Resolver {
160161
)
161162
)
162163
return true
163-
// Canvas operations are specific; ambiguous DOM methods require a typed/ref receiver
164-
// or another established DOM operation in the same function.
164+
/** Canvas operations are specific; ambiguous DOM methods require a typed/ref receiver or another established DOM operation in the same function. */
165165
if (
166166
/^(?:fillRect|strokeRect|drawImage|fillText|strokeText|addColorStop|getContext)$/.test(method)
167167
)
@@ -281,6 +281,28 @@ export class Resolver {
281281
}
282282
}
283283

284+
/** Conditions around writes can change the visible collection even when each value is unchanged. */
285+
private mutationValue(write: NodePath, file: string, depth: number): Data {
286+
const conditions: Data[] = []
287+
for (
288+
let parent = write.parentPath;
289+
parent && !parent.isFunction();
290+
parent = parent.parentPath
291+
) {
292+
if (
293+
parent.isIfStatement() ||
294+
parent.isConditionalExpression() ||
295+
parent.isWhileStatement() ||
296+
parent.isDoWhileStatement() ||
297+
parent.isForStatement()
298+
)
299+
conditions.push(this.value(child(parent, 'test'), file, depth + 1))
300+
if (parent.isForOfStatement() || parent.isForInStatement())
301+
conditions.push(this.value(child(parent, 'right'), file, depth + 1))
302+
}
303+
return { value: this.value(write, file, depth + 1), conditions }
304+
}
305+
284306
private currentFile = ''
285307

286308
/** Select a property before expanding siblings, including createEnv's schema convention. */
@@ -298,8 +320,18 @@ export class Resolver {
298320
return this.selected(child(path, 'expression'), key, file, depth + 1, seen)
299321
if (path.isIdentifier()) {
300322
const binding = path.scope.getBinding(path.node.name)
301-
if (binding?.constant && binding.path.isVariableDeclarator())
302-
return this.selected(child(binding.path, 'init'), key, file, depth + 1, seen)
323+
if (binding?.constant && binding.path.isVariableDeclarator()) {
324+
const initial = this.selected(child(binding.path, 'init'), key, file, depth + 1, seen)
325+
const writes = mutations(binding, key)
326+
if (initial !== undefined && writes.length) {
327+
this.unresolved.add('Writes to this object property feed rendering')
328+
return {
329+
$property: initial,
330+
writes: writes.map((write) => this.mutationValue(write, file, depth + 1)),
331+
}
332+
}
333+
return initial
334+
}
303335
if (binding?.path.isImportSpecifier() || binding?.path.isImportDefaultSpecifier()) {
304336
const declaration = binding.path.parentPath
305337
if (declaration.isImportDeclaration()) {
@@ -459,8 +491,7 @@ export class Resolver {
459491
t.traverseFast(path.node, (node) => {
460492
if (t.isJSXElement(node) || t.isJSXFragment(node)) renders = true
461493
})
462-
// JSX definitions are extracted at their own source. Expanding component bodies
463-
// again at every reference duplicates evidence and confuses refactors with prop changes.
494+
/** JSX definitions are extracted at their own source. Expanding component bodies again at every reference duplicates evidence and confuses refactors with prop changes. */
464495
if (renders) return { $renderFunction: { file, symbol: symbolName(path) } }
465496
const body = child(path, 'body')
466497
if (!body.isBlockStatement())
@@ -803,6 +834,14 @@ export class Resolver {
803834
writes: binding.constantViolations.map((violation) => fingerprint(violation.node)),
804835
}
805836
}
837+
const writes = mutations(binding)
838+
if (writes.length && binding.path.isVariableDeclarator()) {
839+
this.unresolved.add('Collection or object mutations feed this visual input')
840+
return {
841+
$state: this.value(child(binding.path, 'init'), file, depth + 1),
842+
writes: writes.map((write) => this.mutationValue(write, file, depth + 1)),
843+
}
844+
}
806845
const bound = binding.path
807846
const parameter = this.parameter(bound, node.name, file, depth)
808847
if (parameter !== undefined) return parameter
@@ -934,6 +973,8 @@ export class Resolver {
934973
return { $condition: test, then: read('consequent'), else: read('alternate') }
935974
}
936975
if (t.isFunction(node)) return this.functionValue(path, file, depth)
976+
if (t.isAssignmentExpression(node))
977+
return { $assignment: node.operator, target: fingerprint(node.left), value: read('right') }
937978
if (t.isNewExpression(node)) return { $new: read('callee'), arguments: readList('arguments') }
938979
if (t.isTaggedTemplateExpression(node)) return { $tag: read('tag'), template: read('quasi') }
939980
if (t.isAwaitExpression(node)) return { $await: read('argument') }

scripts/design-diff/tests/precision.test.ts

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -299,3 +299,48 @@ it('preserves value/type names shared by generated schema declarations', async (
299299
.some((reason) => /parser|extraction failed/i.test(reason))
300300
).toBe(false)
301301
})
302+
303+
it.each([
304+
(colour: string) =>
305+
`export function colours(){const values=[];values.push('${colour}');return values}`,
306+
(colour: string) =>
307+
`export function colours(){const values={colour:'red'};values.colour='${colour}';return values.colour}`,
308+
])('retains writes to const collections feeding rendering', async (source) => {
309+
const report = await compareFiles(
310+
{
311+
[data]: source('red'),
312+
[view]: 'import {colours} from "./data";export const Page=()=> <Panel colours={colours()}/>',
313+
},
314+
{ [data]: source('blue') },
315+
settings
316+
)
317+
expect(report.flagged).toBe(true)
318+
})
319+
320+
it('retains conditions around collection writes', async () => {
321+
const source = (enabled: boolean) =>
322+
`export function colours(){const values=[];if(${enabled})values.push('red');return values}`
323+
const report = await compareFiles(
324+
{
325+
[data]: source(true),
326+
[view]: 'import {colours} from "./data";export const Page=()=> <Panel colours={colours()}/>',
327+
},
328+
{ [data]: source(false) },
329+
settings
330+
)
331+
expect(report.flagged).toBe(true)
332+
})
333+
334+
it('isolates unrelated mutable object fields such as telemetry warmup state', async () => {
335+
const source = (warm: boolean) =>
336+
`const state={client:null,warmup:false};state.client=connect();state.warmup=${warm};export function client(){return state.client}`
337+
const report = await compareFiles(
338+
{
339+
[data]: source(false),
340+
[view]: 'import {client} from "./data";export const Page=()=> <Panel client={client()}/>',
341+
},
342+
{ [data]: source(true) },
343+
settings
344+
)
345+
expect(report.flagged).toBe(false)
346+
})

0 commit comments

Comments
 (0)