Skip to content

Commit 8859f66

Browse files
committed
fix(hooks): actionable messages, guarded async flip, specific native logging
Remaining small items from the second cold review: - required-mode throws no longer leak the internal hook name (useViewModelInstanceAsync: → useViewModelInstance:), and a null source gets its own message pointing at the upstream hook's error instead of 'ensure the source has a valid ViewModel' - flipping the async param between renders now logs an actionable console.error before React's cryptic hooks-order invariant fires - the VM-less normalization on the new backends logs what it swallows (RiveLog.d) and propagates cancellation instead of eating it - e2e: expectDefined also rejects null (its NonNullable claim was unsound), the resolve-to-null test pins the stable 'not found' message, and android-experimental skips announce themselves - JSDoc: the required example now states that required-narrowing needs a non-nullable source type (nullable sources resolve to the standard overload; the runtime throw still applies)
1 parent 8430822 commit 8859f66

6 files changed

Lines changed: 134 additions & 13 deletions

File tree

android/src/new/java/com/margelo/nitro/rive/HybridRiveFile.kt

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -103,10 +103,16 @@ class HybridRiveFile(
103103
// getDefaultViewModelInfo throws when the artboard has no default
104104
// ViewModel — a normal state for VM-less files (issue #189 fixture), not
105105
// a failure. Resolve null like the legacy backend so callers get the
106-
// documented "no ViewModel" result instead of a raw error.
106+
// documented "no ViewModel" result — but propagate cancellation and log
107+
// what was swallowed so a genuine failure (disposed file, dead command
108+
// queue) isn't a silently blank screen.
107109
val vmInfo = try {
108110
file.getDefaultViewModelInfo(artboard)
111+
} catch (e: kotlinx.coroutines.CancellationException) {
112+
runCatching { artboard.close() }
113+
throw e
109114
} catch (e: Exception) {
115+
RiveLog.d(TAG, "defaultArtboardViewModel: resolving null — getDefaultViewModelInfo failed: ${e.message}")
110116
runCatching { artboard.close() }
111117
return null
112118
}

example/__tests__/useViewModelInstance-async-e2e.harness.tsx

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,10 @@ const NO_DEFAULT_VM = require('../assets/rive/nodefaultbouncing.riv');
3232
const BOUNCING_BALL = require('../assets/rive/bouncing_ball.riv');
3333

3434
function expectDefined<T>(value: T): asserts value is NonNullable<T> {
35+
// toBeDefined() alone passes for null, which would betray the NonNullable
36+
// type claim this helper makes.
3537
expect(value).toBeDefined();
38+
expect(value).not.toBeNull();
3639
}
3740

3841
async function loadFile() {
@@ -348,6 +351,9 @@ describe('useViewModelInstance async: instance creation failure', () => {
348351
await waitFor(() => expect(ctx.isLoading).toBe(false), { timeout: 5000 });
349352
expect(ctx.instance).toBeNull();
350353
expectDefined(ctx.error);
354+
// The stable message, not a leaked raw native diagnostic.
355+
expect(ctx.error.message).toContain('not found');
356+
expect(ctx.error.message).not.toContain('Could not create');
351357
cleanup();
352358
});
353359
});
@@ -415,7 +421,12 @@ describe('useViewModelInstance async: RiveViewRef source', () => {
415421
const isAndroidExperimental =
416422
Platform.OS === 'android' &&
417423
RiveFileFactory.getBackend() === 'experimental';
418-
if (isAndroidExperimental) return;
424+
if (isAndroidExperimental) {
425+
console.warn(
426+
'SKIP: android-experimental — getViewModelInstance() does not expose the auto-bound VMI yet'
427+
);
428+
return;
429+
}
419430

420431
const file = await RiveFileFactory.fromSource(BOUNCING_BALL, undefined);
421432
const ctx = createCtx();
@@ -430,7 +441,12 @@ describe('useViewModelInstance async: RiveViewRef source', () => {
430441
const isAndroidExperimental =
431442
Platform.OS === 'android' &&
432443
RiveFileFactory.getBackend() === 'experimental';
433-
if (isAndroidExperimental) return;
444+
if (isAndroidExperimental) {
445+
console.warn(
446+
'SKIP: android-experimental — getViewModelInstance() does not expose the auto-bound VMI yet'
447+
);
448+
return;
449+
}
434450

435451
const file = await RiveFileFactory.fromSource(BOUNCING_BALL, undefined);
436452
const ctx = createCtx();

ios/new/HybridRiveFile.swift

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -92,11 +92,18 @@ class HybridRiveFile: HybridRiveFileSpec {
9292
// getDefaultViewModelInfo throws when the artboard has no default
9393
// ViewModel — a normal state for VM-less files (issue #189 fixture), not
9494
// a failure. Resolve nil like the legacy backend so callers get the
95-
// documented "no ViewModel" result instead of a raw error.
96-
guard let vmInfo = try? await file.getDefaultViewModelInfo(for: artboard) else {
95+
// documented "no ViewModel" result — but propagate cancellation and log
96+
// what was swallowed so a genuine failure (disposed file, dead worker)
97+
// isn't a silently blank screen.
98+
do {
99+
let vmInfo = try await file.getDefaultViewModelInfo(for: artboard)
100+
return HybridViewModel(file: file, vmName: vmInfo.viewModelName, worker: worker)
101+
} catch is CancellationError {
102+
throw CancellationError()
103+
} catch {
104+
RiveLog.d("RiveFile", "defaultArtboardViewModel: resolving nil — getDefaultViewModelInfo failed: \(error)")
97105
return nil
98106
}
99-
return HybridViewModel(file: file, vmName: vmInfo.viewModelName, worker: worker)
100107
}
101108

102109
// Deprecated: Use defaultArtboardViewModelAsync instead

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

Lines changed: 72 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -687,9 +687,79 @@ describe('useViewModelInstance async - required', () => {
687687
);
688688

689689
await waitFor(() => expect(onError).toHaveBeenCalled());
690-
expect((onError.mock.calls[0][0] as Error).message).toContain(
691-
'NonExistent'
690+
const message = (onError.mock.calls[0][0] as Error).message;
691+
expect(message).toContain('NonExistent');
692+
// Users called useViewModelInstance — the internal hook name must not leak.
693+
expect(message).toContain('useViewModelInstance:');
694+
expect(message).not.toContain('useViewModelInstanceAsync');
695+
consoleError.mockRestore();
696+
});
697+
698+
it('names the absent source (not the ViewModel) when required throws for a null source', async () => {
699+
const consoleError = jest
700+
.spyOn(console, 'error')
701+
.mockImplementation(() => {});
702+
const onError = jest.fn();
703+
704+
function NullSourceProbe() {
705+
useViewModelInstance(null as RiveFile | null, {
706+
async: true,
707+
required: true,
708+
});
709+
return null;
710+
}
711+
712+
render(
713+
<ErrorBoundary onError={onError}>
714+
<NullSourceProbe />
715+
</ErrorBoundary>
692716
);
717+
718+
await waitFor(() => expect(onError).toHaveBeenCalled());
719+
const message = (onError.mock.calls[0][0] as Error).message;
720+
// A null source means the file/view failed upstream — pointing users at
721+
// "Ensure the source has a valid ViewModel" sends them the wrong way.
722+
expect(message).toContain('useViewModelInstance:');
723+
expect(message).toMatch(/source is null/i);
724+
expect(message).toMatch(/useRiveFile|useRive/);
725+
consoleError.mockRestore();
726+
});
727+
});
728+
729+
describe('useViewModelInstance - async flag constancy guard', () => {
730+
it('reports an actionable error when async changes between renders', async () => {
731+
const consoleError = jest
732+
.spyOn(console, 'error')
733+
.mockImplementation(() => {});
734+
const defaultInstance = createMockViewModelInstance();
735+
// The initial render takes the sync path, so the mock must speak the
736+
// sync API too (the shared factories only stub the *Async surface).
737+
const mockRiveFile = {
738+
...createMockRiveFile({
739+
defaultViewModel: createMockViewModel({ defaultInstance }),
740+
}),
741+
defaultArtboardViewModel: jest.fn(() => ({
742+
dispose: jest.fn(),
743+
createDefaultInstance: jest.fn(() => defaultInstance),
744+
createInstanceByName: jest.fn(),
745+
createInstance: jest.fn(),
746+
})),
747+
} as unknown as RiveFile;
748+
749+
const { rerender } = renderHook(
750+
({ isAsync }: { isAsync: boolean }) =>
751+
useViewModelInstance(mockRiveFile, { async: isAsync }),
752+
{ initialProps: { isAsync: false } }
753+
);
754+
755+
// Flipping the flag switches hook implementations — React will throw its
756+
// generic hooks-order invariant; the hook must first explain why.
757+
expect(() => rerender({ isAsync: true })).toThrow();
758+
expect(
759+
consoleError.mock.calls.some((c) =>
760+
String(c[0]).includes('`async` param changed between renders')
761+
)
762+
).toBe(true);
693763
consoleError.mockRestore();
694764
});
695765
});

src/hooks/useViewModelInstance.ts

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -292,6 +292,9 @@ export type UseViewModelInstanceRequiredResult =
292292
* ```tsx
293293
* // With required: true (throws once resolved to null, use with Error Boundary).
294294
* // Note: instance is still `undefined` while loading — guard on isLoading.
295+
* // The required-narrowed return type applies only when the source's TYPE is
296+
* // non-nullable; with a nullable source (e.g. straight from useRiveFile) the
297+
* // call resolves to the standard overload — the runtime throw still applies.
295298
* const { instance, isLoading } = useViewModelInstance(riveFile, { async: true, required: true });
296299
* ```
297300
*
@@ -399,7 +402,18 @@ export function useViewModelInstance(
399402
const isAsync = params?.async ?? false;
400403
// The flag selects between two hook implementations (different hook
401404
// orders), so it must stay constant for the lifetime of the component —
402-
// documented on the param.
405+
// documented on the param. React's own failure for a flip is the cryptic
406+
// "Rendered more hooks than during the previous render"; explain first.
407+
const initialAsyncRef = useRef(isAsync);
408+
if (initialAsyncRef.current !== isAsync) {
409+
console.error(
410+
'useViewModelInstance: the `async` param changed between renders ' +
411+
`(${String(initialAsyncRef.current)}${String(isAsync)}). It selects ` +
412+
'between two hook implementations, so it must stay constant for the ' +
413+
'lifetime of the component; remount (e.g. change `key`) to switch modes.'
414+
);
415+
initialAsyncRef.current = isAsync;
416+
}
403417
if (isAsync) {
404418
// eslint-disable-next-line react-hooks/rules-of-hooks
405419
return useViewModelInstanceAsync(
@@ -469,8 +483,8 @@ function useViewModelInstanceSync(
469483
// async path.
470484
if (required) {
471485
throw new Error(
472-
'useViewModelInstance: Failed to get ViewModelInstance. ' +
473-
'Ensure the source has a valid ViewModel and instance available.'
486+
'useViewModelInstance: source is null — the file or view failed to ' +
487+
"resolve upstream (if it comes from useRiveFile or useRive, check that hook's error)."
474488
);
475489
}
476490
return { instance: null, isLoading: false, error: null };

src/hooks/useViewModelInstanceAsync.ts

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -425,10 +425,18 @@ export function useViewModelInstanceAsync(
425425
}, [source, instanceName, artboardName, viewModelName, useNew]);
426426

427427
if (required && result.instance === null && !result.isLoading) {
428+
// The public entry point is useViewModelInstance — don't leak this
429+
// internal hook's name into user-facing errors.
430+
if (source === null) {
431+
throw new Error(
432+
'useViewModelInstance: source is null — the file or view failed to ' +
433+
"resolve upstream (if it comes from useRiveFile or useRive, check that hook's error)."
434+
);
435+
}
428436
throw new Error(
429437
result.error
430-
? `useViewModelInstanceAsync: ${result.error.message}`
431-
: 'useViewModelInstanceAsync: Failed to get ViewModelInstance. ' +
438+
? `useViewModelInstance: ${result.error.message}`
439+
: 'useViewModelInstance: Failed to get ViewModelInstance. ' +
432440
'Ensure the source has a valid ViewModel and instance available.'
433441
);
434442
}

0 commit comments

Comments
 (0)