Skip to content

Commit 9360ed4

Browse files
serpentbladeclaude
andcommitted
fix(target-react): transitively hoist deeply-indirected module-lets to useRef (Plan 04-04)
CI on 248f2cf went red: two React rete-flow VR tests (undo + reconnect) failed deterministically — undo no-op'd, leaving the post-drag value in place. Root cause was a latent emitter gap exposed by a663b0d's correct live-read `$expose` rewrite. `hoistModuleLet` lifts a module-scoped `let X = init` to a per-instance `useRef` only when it is reachable from a lifecycle hook DIRECTLY or via a SINGLE helper level (cases a/b). FlowCanvas's `historyStack`/`redoStack` are reached only transitively ($onMount → event handler → pushHistory → pushHistorySnapshot → stack), so they fell through to case (c) and stayed as a per-render `let [] ` — reset to a fresh empty array every render. While every consumer of the stacks was a first-render closure (handlers bound once in $onMount; the old `$expose` handle captured at mount via `[]` deps) the bug was dormant — they all shared render-0's array. The live-read handle made the external `undo`/`redo` dispatch to the LATEST render's closure, which read the freshly-emptied stack → undo had nothing to pop. Fix: take the TRANSITIVE closure over the top-level helper call graph (fixpoint, cycle-safe) and follow helper calls inside lifecycle/watcher bodies, so a let reached through any depth of helper indirection hoists. This is the deferred Plan 04-04 promotion. Regenerating all 14 React leaves changes exactly four — the components with the same deep-indirection pattern, all now correctly stabilised: - rete/FlowCanvas: historyStack, redoStack (the reported regression) - cropper/Cropper: cropReady round-trip guard - data-table/DataTable: programmatic echo-guard, selectAllBox - maplibre/Layer: ctx Each was a per-render-reset guard that is now a stable useRef — latent bugs of the same class, hardened. Verified: 216/216 turbo test+typecheck tasks green; dist-parity unchanged; the 2 previously-failing rete-flow tests + all 19 rete-flow React VR cells pass locally. Test 3 in hoistModuleLet.test.ts flipped from asserting the old conservative no-hoist to asserting the new transitive hoist. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent ed7dd32 commit 9360ed4

6 files changed

Lines changed: 156 additions & 159 deletions

File tree

packages/targets/react/src/__tests__/hoistModuleLet.test.ts

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -240,12 +240,14 @@ $onMount(setup);
240240
expect(out).toMatch(/\{\s*engine:\s*engine\.current\s*\}/);
241241
});
242242

243-
it('Test 3 (deeply-nested): when a let is referenced via 2-level helper indirection it is NOT auto-hoisted (conservative)', () => {
243+
it('Test 3 (deeply-nested): a let referenced via 2-level helper indirection IS auto-hoisted (Plan 04-04 transitive promotion)', () => {
244244
// Synthetic: let X referenced inside `helperA`, `helperA` called from
245-
// `helperB`, and `helperB` is the lifecycle setup. Per the spike
246-
// pseudocode the conservative rule covers (a) and (b) only — deeper
247-
// chains fall through with no hoist + no diag (Plan 04-04 may add
248-
// ROZ523 promotion for this in a follow-up).
245+
// `helperB`, and `helperB` is the lifecycle setup. The pass now takes the
246+
// TRANSITIVE closure over the top-level helper call graph, so `nested`
247+
// (reached as $onMount → helperB → helperA → nested) hoists to `useRef`.
248+
// Before this promotion it stayed a per-render `let nested = 0` — reset to
249+
// 0 every render — which silently broke any closure that captured a later
250+
// render's copy (the live-read `$expose` undo-stack regression, 2026-06-18).
249251
const src = `
250252
let nested = 0;
251253
const helperA = () => { nested = 1; };
@@ -279,10 +281,14 @@ $onMount(helperB);
279281
sourceLoc: { start: 0, end: 0 },
280282
};
281283
const { hoisted } = hoistModuleLet(program, syntheticIR);
282-
// Conservative — helperB references no let directly, helperA does but
283-
// helperA isn't a lifecycle setup. So `nested` stays untouched.
284-
expect(hoisted).toHaveLength(0);
284+
// Transitive: helperB calls helperA, which writes `nested` → the closure
285+
// reaches `nested`, so it hoists.
286+
expect(hoisted).toHaveLength(1);
287+
expect(hoisted[0]!.name).toBe('nested');
285288
const out = generate(program).code;
286-
expect(out).toContain('let nested');
289+
// The `let nested = 0` declaration is removed (emitReact synthesises the
290+
// `useRef(0)`), and every reference is rewritten to `nested.current`.
291+
expect(out).not.toContain('let nested');
292+
expect(out).toMatch(/nested\.current\s*=\s*1/);
287293
});
288294
});

packages/targets/react/src/rewrite/hoistModuleLet.ts

Lines changed: 82 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,10 @@
22
* hoistModuleLet — Plan 04-02 Task 2 (Wave 0 spike resolution).
33
*
44
* Detects module-scoped `let X = init` declarations referenced from a
5-
* LifecycleHook setup body (directly or via one-level helper indirection),
6-
* auto-hoists them to a `useRef(init)` inside the component body, and
7-
* rewrites every reference (read AND write) to `X.current`.
5+
* LifecycleHook setup body (directly, or via ANY depth of top-level helper
6+
* call indirection — Plan 04-04 transitive promotion), auto-hoists them to a
7+
* `useRef(init)` inside the component body, and rewrites every reference
8+
* (read AND write) to `X.current`.
89
*
910
* Spike outcome (04-02-SPIKE.md): Modal.rozie's `let savedBodyOverflow = ''`
1011
* referenced by `lockScroll`/`unlockScroll` (top-level arrows passed
@@ -16,12 +17,18 @@
1617
* 2. Top-level helpers (const X = arrow|fn OR function X) → which
1718
* module-lets they reference
1819
* 3. For each LifecycleHook:
19-
* (a) setup is arrow/fn whose body references a module-let → hoist
20-
* (b) setup OR cleanup is Identifier matching a top-level helper that
21-
* references a module-let → hoist
22-
* (c) deeper indirection / unknown — leave UNHOISTED (no diagnostic
23-
* in v1; user code remains as `let X = ...` and survives via the
24-
* residual top-level Program. Plan 04-04 may add ROZ523 promotion.)
20+
* (a) setup is arrow/fn whose body references a module-let (directly or
21+
* by calling a helper chain that does) → hoist
22+
* (b) setup OR cleanup is Identifier matching a top-level helper whose
23+
* TRANSITIVE call closure references a module-let → hoist
24+
* (c) no reachable reference — leave UNHOISTED. (Plan 04-04 promoted the
25+
* former one-level-only limit to the full transitive closure over the
26+
* top-level helper call graph, so a let reached as
27+
* `$onMount → handler → pushHistory → pushHistorySnapshot → stack`
28+
* now hoists. Before, it stayed a per-render `let` — re-initialised
29+
* to its `init` on every render — which silently broke any closure
30+
* that captured a later render's copy, e.g. the live-read `$expose`
31+
* handle reading an undo stack that was empty again.)
2532
*
2633
* For hoisted lets:
2734
* - REMOVE the original `let X = init` from cloned Program top level
@@ -201,8 +208,10 @@ export function hoistModuleLet(program: File, ir: IRComponent): HoistResult {
201208

202209
const moduleLetNames = new Set(moduleLets.keys());
203210

204-
// 2. Build map: top-level helper name → set of module-let names it references.
205-
const topLevelHelpers = new Map<string, Set<string>>();
211+
// 2. Catalogue EVERY top-level helper (name → body), independent of whether
212+
// it directly touches a module-let — a helper that only CALLS another
213+
// helper still participates in the transitive reachability graph below.
214+
const helperBodies = new Map<string, t.Node>();
206215
for (const stmt of program.program.body) {
207216
let helperName: string | null = null;
208217
let body: t.Node | null = null;
@@ -226,9 +235,51 @@ export function hoistModuleLet(program: File, ir: IRComponent): HoistResult {
226235
body = stmt;
227236
}
228237
if (!helperName || !body) continue;
238+
helperBodies.set(helperName, body);
239+
}
240+
const helperNames = new Set(helperBodies.keys());
229241

230-
const refs = collectIdentifierRefs(body, moduleLetNames);
231-
if (refs.size > 0) topLevelHelpers.set(helperName, refs);
242+
// 2b. For each helper compute (i) the module-lets it references DIRECTLY and
243+
// (ii) the other helpers it CALLS. Then take the fixpoint over the call
244+
// graph so `helperTransitiveLets[H]` holds every module-let reachable
245+
// from H through ANY chain of helper calls. This is the Plan 04-04
246+
// promotion: the original pass only resolved ONE level of helper
247+
// indirection (case (b)), so a let reached as
248+
// `$onMount → handler → pushHistory → pushHistorySnapshot → historyStack`
249+
// fell through to case (c) and was left as a per-render `let` — fresh
250+
// `[]` every render. Any closure that captured a LATER render (e.g. the
251+
// live-read `$expose` handle) then read an empty stack. Transitive
252+
// hoisting lifts the whole chain to `useRef`, restoring shared identity.
253+
const helperDirectLets = new Map<string, Set<string>>();
254+
const helperCalls = new Map<string, Set<string>>();
255+
for (const [name, body] of helperBodies) {
256+
helperDirectLets.set(name, collectIdentifierRefs(body, moduleLetNames));
257+
const calls = collectIdentifierRefs(body, helperNames);
258+
calls.delete(name); // a self-reference adds nothing to the closure
259+
helperCalls.set(name, calls);
260+
}
261+
const helperTransitiveLets = new Map<string, Set<string>>();
262+
for (const name of helperNames) {
263+
helperTransitiveLets.set(name, new Set(helperDirectLets.get(name)));
264+
}
265+
// Fixpoint iteration — terminates because the let universe is finite and
266+
// each pass only adds. Cycle-safe (a helper cycle just shares one set).
267+
let grew = true;
268+
while (grew) {
269+
grew = false;
270+
for (const name of helperNames) {
271+
const acc = helperTransitiveLets.get(name)!;
272+
for (const callee of helperCalls.get(name)!) {
273+
const calleeLets = helperTransitiveLets.get(callee);
274+
if (!calleeLets) continue;
275+
for (const l of calleeLets) {
276+
if (!acc.has(l)) {
277+
acc.add(l);
278+
grew = true;
279+
}
280+
}
281+
}
282+
}
232283
}
233284

234285
// 3. Walk lifecycle hooks AND watchers → which lets are referenced via
@@ -247,11 +298,21 @@ export function hoistModuleLet(program: File, ir: IRComponent): HoistResult {
247298
classifyExpr(wh.callback);
248299
}
249300

301+
/** Pull every let a body reaches: its DIRECT refs plus the transitive lets
302+
* of every helper it calls (at any nesting depth inside the body). */
303+
function addBodyReachableLets(body: t.Node): void {
304+
for (const r of collectIdentifierRefs(body, moduleLetNames)) referencedLets.add(r);
305+
for (const h of collectIdentifierRefs(body, helperNames)) {
306+
const lets = helperTransitiveLets.get(h);
307+
if (lets) for (const l of lets) referencedLets.add(l);
308+
}
309+
}
310+
250311
function classifyExpr(expr: t.Expression | t.BlockStatement): void {
251-
// (a) DIRECT — arrow/fn expression body references a let.
312+
// (a) DIRECT — arrow/fn expression body references a let (or calls a
313+
// helper chain that does).
252314
if (t.isArrowFunctionExpression(expr) || t.isFunctionExpression(expr)) {
253-
const refs = collectIdentifierRefs(expr, moduleLetNames);
254-
for (const r of refs) referencedLets.add(r);
315+
addBodyReachableLets(expr);
255316
return;
256317
}
257318
// (a') DIRECT — BlockStatement body (post-`extractCleanupReturn` lift,
@@ -262,13 +323,13 @@ export function hoistModuleLet(program: File, ir: IRComponent): HoistResult {
262323
// React render, breaking the let's "module-scoped scratch state"
263324
// semantics (e.g. FullCalendar's `let suppressViewSync = false`).
264325
if (t.isBlockStatement(expr)) {
265-
const refs = collectIdentifierRefs(expr, moduleLetNames);
266-
for (const r of refs) referencedLets.add(r);
326+
addBodyReachableLets(expr);
267327
return;
268328
}
269-
// (b) ONE-LEVEL HELPER — Identifier referring to a top-level helper.
270-
if (t.isIdentifier(expr) && topLevelHelpers.has(expr.name)) {
271-
const refs = topLevelHelpers.get(expr.name)!;
329+
// (b) HELPER — Identifier referring to a top-level helper. Now resolves
330+
// the helper's TRANSITIVE lets, not just its direct ones.
331+
if (t.isIdentifier(expr) && helperTransitiveLets.has(expr.name)) {
332+
const refs = helperTransitiveLets.get(expr.name)!;
272333
for (const r of refs) referencedLets.add(r);
273334
return;
274335
}

packages/ui/cropper/packages/react/src/Cropper.tsx

Lines changed: 3 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,7 @@ const Cropper = forwardRef<CropperHandle, CropperProps>(function Cropper(_props:
109109
})();
110110
const imgEl = useRef<any>(null);
111111
const instance = useRef<any>(null);
112+
const cropReady = useRef(false);
112113
const [data, setData] = useControllableState({
113114
value: props.data,
114115
defaultValue: props.defaultData ?? undefined,
@@ -121,22 +122,6 @@ const Cropper = forwardRef<CropperHandle, CropperProps>(function Cropper(_props:
121122
const _watch3First = useRef(true);
122123
const _watch4First = useRef(true);
123124

124-
// Gate that suppresses the engine's SETUP-time `crop` events from writing the
125-
// two-way `$model.data`. Cropper fires an initial `crop` with its OWN default box
126-
// (autoCropArea) BEFORE the `ready` callback runs, and the `setData($props.data)`
127-
// inside `ready` fires another. Writing those transient engine-internal boxes to
128-
// `$model.data` is wrong — and on unified-model targets (Vue defineModel / Svelte
129-
// $bindable / Angular model() signal, where the model read and write share ONE
130-
// local) the pre-ready write CLOBBERS the very `$props.data` that `ready` then
131-
// reads, so the consumer's initial `:data` crop box is lost and the default box is
132-
// applied instead. (React/Solid read the external prop and Lit's property binding
133-
// is controlled, so the write doesn't change their read — which is why only the
134-
// template-emit family regressed.) We flip this true at the END of `ready`, after
135-
// the initial box is applied, so only genuine post-init user crops drive the model.
136-
let cropReady = false;
137-
138-
// pure crop-box equality (rounded px + exact transform) — no sigils, safe at top
139-
// level. The round-trip guard that stops the setData→crop→$model.data→$watch loop.
140125
function sameData(a: any, b: any) {
141126
if (!a || !b) return false;
142127
return Math.round(a.x) === Math.round(b.x) && Math.round(a.y) === Math.round(b.y) && Math.round(a.width) === Math.round(b.width) && Math.round(a.height) === Math.round(b.height) && a.rotate === b.rotate && a.scaleX === b.scaleX && a.scaleY === b.scaleY;
@@ -176,7 +161,7 @@ const Cropper = forwardRef<CropperHandle, CropperProps>(function Cropper(_props:
176161
// exactly ONCE here (after `$props.data` has been read for setData — no clobber),
177162
// then open the gate so genuine post-init user crops drive the model.
178163
setData(instance.current.getData());
179-
cropReady = true;
164+
cropReady.current = true;
180165
_rozieProp_onReady && _rozieProp_onReady();
181166
},
182167
cropstart: (e: any) => _rozieProp_onCropstart && _rozieProp_onCropstart({
@@ -194,7 +179,7 @@ const Cropper = forwardRef<CropperHandle, CropperProps>(function Cropper(_props:
194179
// the `setData($props.data)` echo). Propagating them would (a) emit a spurious
195180
// pre-init `crop` and (b) on unified-model targets clobber the consumer's
196181
// initial `:data`. Genuine user crops fire after `cropReady`.
197-
if (!cropReady) return;
182+
if (!cropReady.current) return;
198183
_rozieProp_onCrop && _rozieProp_onCrop(e.detail);
199184
if (e.detail) setData(e.detail);
200185
},

0 commit comments

Comments
 (0)