Skip to content

Commit 91f7db3

Browse files
vanceingallsclaude
andcommitted
fix(studio): make automationLaneData reviewable, and evict one entry not all
The cache key held a literal NUL byte instead of its escape, so git classified the whole module as binary: it landed as `Bin 0 -> 2677 bytes` with zero diffable lines, invisible to review, to grep, and to any textual merge. The escape is behaviour-identical. With the file readable, two things in it needed fixing. Eviction cleared the entire map. Clearing changes the identity of every lane's automation at once, and a lane compares its drag draft against that identity — so one unrelated element arriving at the limit would release an in-progress drag and snap the point back. It now drops the oldest entry, and a hit is re-inserted so it counts as recently used. Nothing tested this module, which is what let the binary blob through. Now covered: identity stability, re-parsing when the chain changes but the automation text does not, a hot entry surviving 40 evictions, and an unreadable attribute reading as nothing. The geometry module's exports are ignored for dead-code while its consumer sits one PR upstack, following the convention already used for the fast-capture stack. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 812994e commit 91f7db3

3 files changed

Lines changed: 82 additions & 0 deletions

File tree

.fallowrc.jsonc

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -160,6 +160,22 @@
160160
"file": "packages/core/src/audio/audioFxGraph.ts",
161161
"exports": ["ensureAudioFxWorklets"],
162162
},
163+
// automationLaneGeometry is the bottom of the audio-automation stack: its
164+
// consumer is the lane component one PR upstack, so a per-PR audit diffing
165+
// against the merge base sees these as unused. Consumed for real once the
166+
// stack merges; safe to drop this entry then.
167+
{
168+
"file": "packages/studio/src/player/components/automationLaneGeometry.ts",
169+
"exports": [
170+
"POINT_MERGE_SEC",
171+
"GRAB_PX",
172+
"DRAW_SAMPLES",
173+
"PAD_X",
174+
"formatValue",
175+
"laneFor",
176+
"withLane",
177+
],
178+
},
163179
// drawElementService is the bottom of the fast-capture Graphite stack
164180
// (#1917): its consumers (frameCapture in #1919) land two PRs upstack, so
165181
// a per-PR audit diffing against the merge base sees these exports as
Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
// @vitest-environment happy-dom
2+
import { describe, expect, it } from "vitest";
3+
import { elementAutomation, elementFxChain } from "./automationLaneData";
4+
import type { TimelineElement } from "../store/timelineElement";
5+
6+
const el = (over: Partial<TimelineElement> = {}): TimelineElement => ({
7+
id: "bgm",
8+
key: "bgm",
9+
tag: "audio",
10+
start: 0,
11+
duration: 10,
12+
track: 10,
13+
...over,
14+
});
15+
16+
const CHAIN = JSON.stringify({
17+
version: 1,
18+
nodes: [{ type: "lowpass", id: "n1", params: { frequency: 400, q: 0.9, poles: "2" } }],
19+
});
20+
const LANE = JSON.stringify({
21+
version: 1,
22+
lanes: [{ target: "fx.n1.frequency", points: [{ t: 0, v: 400 }] }],
23+
});
24+
25+
describe("automationLaneData", () => {
26+
it("returns the same object for the same attributes", () => {
27+
// The lane compares its drag draft against this identity; a fresh object per
28+
// playhead tick would drop the drag.
29+
const a = elementAutomation(el({ automation: LANE, fxChain: CHAIN }));
30+
const b = elementAutomation(el({ automation: LANE, fxChain: CHAIN }));
31+
expect(a).toBe(b);
32+
expect(elementFxChain(el({ fxChain: CHAIN }))).toBe(elementFxChain(el({ fxChain: CHAIN })));
33+
});
34+
35+
it("re-parses when the chain changes even though the automation text did not", () => {
36+
const withChain = elementAutomation(el({ automation: LANE, fxChain: CHAIN }));
37+
const withoutChain = elementAutomation(el({ automation: LANE }));
38+
expect(withChain.lanes.map((l) => l.target)).toEqual(["fx.n1.frequency"]);
39+
// No chain to resolve against, so the fx lane is dropped rather than drawn.
40+
expect(withoutChain.lanes).toEqual([]);
41+
});
42+
43+
it("keeps a hot entry alive when other elements push the cache past its limit", () => {
44+
// Eviction used to clear the whole map, which changed every lane's identity
45+
// at once and released any drag in progress.
46+
const hot = el({ automation: LANE, fxChain: CHAIN });
47+
const first = elementAutomation(hot);
48+
for (let i = 0; i < 40; i += 1) {
49+
elementAutomation(
50+
el({
51+
automation: JSON.stringify({
52+
version: 1,
53+
lanes: [{ target: "volume", points: [{ t: i, v: 0.5 }] }],
54+
}),
55+
}),
56+
);
57+
elementAutomation(hot);
58+
}
59+
expect(elementAutomation(hot)).toBe(first);
60+
});
61+
62+
it("reads an unreadable attribute as nothing rather than throwing", () => {
63+
expect(elementAutomation(el({ automation: "{nope" })).lanes).toEqual([]);
64+
expect(elementFxChain(el({ fxChain: "{nope" }))).toBeNull();
65+
});
66+
});
720 Bytes
Binary file not shown.

0 commit comments

Comments
 (0)