Skip to content

Commit dec621d

Browse files
committed
fix(hooks): reset during render on source change; main-hop legacy Android getViewModelInstance
Two fixes from the second cold review: - The loading reset moved from the effect into render (adjust-state- during-render): the effect-time reset let React commit one frame pairing the new source with the previous instance at isLoading: false — a mismatch consumer guards can't catch (e.g. <RiveView file={fileB} dataBind={instanceA}/> reaching native) — and then disposed that instance while still committed. New unit test captures committed frames on a source flip and fails on the old behavior (26 mismatched frames). - Legacy Android's HybridRiveView.getViewModelInstance() read controller.stateMachines on the JS thread while the main thread mutates it during startup (issue #297 race class; iOS already hops via MainThread.run). Now reads under runBlocking(Dispatchers.Main.immediate). No deterministic test is possible for the race; the ref-source e2e tests exercise this path on the android-legacy CI job.
1 parent d3b17e3 commit dec621d

3 files changed

Lines changed: 76 additions & 5 deletions

File tree

android/src/legacy/java/com/margelo/nitro/rive/HybridRiveView.kt

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import app.rive.runtime.kotlin.core.Alignment as RiveAlignment
1212
import app.rive.runtime.kotlin.core.errors.*
1313
import android.util.Log
1414
import kotlinx.coroutines.Dispatchers
15+
import kotlinx.coroutines.runBlocking
1516
import kotlinx.coroutines.withContext
1617

1718
fun Variant_HybridViewModelInstanceSpec_DataBindMode_DataBindByName?.toBindData(): BindData {
@@ -115,7 +116,13 @@ class HybridRiveView(val context: ThemedReactContext) : HybridRiveViewSpec() {
115116
}
116117

117118
override fun getViewModelInstance(): HybridViewModelInstanceSpec? {
118-
val viewModelInstance = view.getViewModelInstance() ?: return null
119+
// Read on the main thread: the controller's state-machine list is mutated
120+
// there during startup and the legacy runtime has no internal
121+
// synchronization (issue #297 race class). Mirrors iOS's MainThread.run;
122+
// `immediate` avoids a deadlock when already called on main.
123+
val viewModelInstance =
124+
runBlocking(Dispatchers.Main.immediate) { view.getViewModelInstance() }
125+
?: return null
119126
return HybridViewModelInstance(viewModelInstance)
120127
}
121128

src/hooks/__tests__/useViewModelInstance.async.test.tsx

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -533,6 +533,55 @@ describe('useViewModelInstance async - disposal', () => {
533533
expect(defaultInstance.dispose).toHaveBeenCalled();
534534
});
535535

536+
it('never commits a frame pairing the new source with the old instance', async () => {
537+
// On source change the reset must happen during render, not in the
538+
// effect: otherwise React commits one frame of
539+
// <RiveView file={fileB} dataBind={instanceA} /> (isLoading false, so
540+
// consumer guards can't catch it) and then disposes instanceA while it
541+
// is still the committed dataBind.
542+
const instanceA = createMockViewModelInstance('A');
543+
const fileA = createMockRiveFile({
544+
defaultViewModel: createMockViewModel({ defaultInstance: instanceA }),
545+
});
546+
const instanceB = createMockViewModelInstance('B');
547+
const fileB = createMockRiveFile({
548+
defaultViewModel: createMockViewModel({ defaultInstance: instanceB }),
549+
});
550+
551+
const frames: Array<{
552+
file: RiveFile;
553+
instance: ViewModelInstance | null | undefined;
554+
isLoading: boolean;
555+
}> = [];
556+
function Probe({ file }: { file: RiveFile }) {
557+
const { instance, isLoading } = useViewModelInstance(file, {
558+
async: true,
559+
});
560+
React.useEffect(() => {
561+
frames.push({ file, instance, isLoading });
562+
});
563+
return null;
564+
}
565+
566+
const view = render(<Probe file={fileA} />);
567+
await waitFor(() =>
568+
expect(frames.some((f) => f.instance === instanceA)).toBe(true)
569+
);
570+
571+
await act(async () => {
572+
view.rerender(<Probe file={fileB} />);
573+
await Promise.resolve();
574+
});
575+
await waitFor(() =>
576+
expect(frames.some((f) => f.instance === instanceB)).toBe(true)
577+
);
578+
579+
const mismatched = frames.filter(
580+
(f) => f.file === fileB && f.instance === instanceA
581+
);
582+
expect(mismatched).toEqual([]);
583+
});
584+
536585
it('disposes the previous instance when the source changes', async () => {
537586
const instanceA = createMockViewModelInstance('A');
538587
const fileA = createMockRiveFile({

src/hooks/useViewModelInstanceAsync.ts

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -331,6 +331,25 @@ export function useViewModelInstanceAsync(
331331
const [result, setResult] =
332332
useState<UseViewModelInstanceAsyncResult>(LOADING_RESULT);
333333

334+
// Reset to the loading state during render (not in the effect) when the
335+
// inputs change: an effect-time reset would let React commit one frame
336+
// pairing the new source with the previous (about-to-be-disposed) instance
337+
// at isLoading: false — a mismatch consumer guards cannot catch, and the
338+
// old instance would be disposed while still committed as e.g. a view's
339+
// dataBind. Resetting mid-render makes React re-render before committing.
340+
const [prevDeps, setPrevDeps] = useState<readonly unknown[]>([
341+
source,
342+
instanceName,
343+
artboardName,
344+
viewModelName,
345+
useNew,
346+
]);
347+
const deps = [source, instanceName, artboardName, viewModelName, useNew];
348+
if (deps.some((d, i) => d !== prevDeps[i])) {
349+
setPrevDeps(deps);
350+
setResult((prev) => (prev.isLoading ? prev : LOADING_RESULT));
351+
}
352+
334353
useEffect(() => {
335354
if (source === null) {
336355
// Source resolved to absent/failed rather than pending. `useRiveFile`
@@ -342,10 +361,6 @@ export function useViewModelInstanceAsync(
342361
return;
343362
}
344363

345-
// Reset to the loading state whenever the inputs change so we never expose a
346-
// stale (and about-to-be-disposed) instance from a previous resolution.
347-
setResult((prev) => (prev.isLoading ? prev : LOADING_RESULT));
348-
349364
if (!source) {
350365
// `undefined`: not resolved yet (e.g. the file is still loading).
351366
return;

0 commit comments

Comments
 (0)