Conversation
There was a problem hiding this comment.
Pull request overview
Adds an inactiveBehavior: 'retain' option to @lifecycleBound() to prevent lifecycle-bound graphs from being evicted during React StrictMode effect replays / React 19 Activity pause-resume cycles.
Changes:
- Extend
@lifecycleBound()andObjectGraphwithinactiveBehaviormetadata/accessor. - Wrap injected components with a platform-aware container/ref to distinguish “effect cleanup due to pause” vs “true unmount”.
- Add
jest-domand new integration tests covering retained lifecycle-bound graphs.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| yarn.lock | Locks new test dependency transitive graph (@testing-library/jest-dom). |
| packages/react-obsidian/package.json | Adds @testing-library/jest-dom to devDependencies. |
| packages/react-obsidian/tsconfig.base.json | Includes @testing-library/jest-dom types for TS matcher support. |
| packages/react-obsidian/jest.setup-after-env.js | Registers jest-dom matchers in Jest environment. |
| packages/react-obsidian/src/decorators/LifecycleBound.ts | Adds inactiveBehavior option/metadata to the decorator. |
| packages/react-obsidian/src/graph/ObjectGraph.ts | Exposes inactiveBehavior getter on graph instances. |
| packages/react-obsidian/src/injectors/components/useGraph.ts | Alters cleanup behavior to avoid clearing graphs on “inactive” cleanups. |
| packages/react-obsidian/src/injectors/components/ComponentInjector.tsx | Introduces container ref + container wrapper around injected component output. |
| packages/react-obsidian/src/injectors/components/useGraphContainer.tsx | Implements retain vs passthrough container selection + platform element selection. |
| packages/react-obsidian/src/utils/reactNativeAvailability.ts | Adds runtime RN availability detection helper. |
| packages/react-obsidian/test/fixtures/RetainedLifecycleBoundGraph.ts | Adds a lifecycle-bound graph fixture configured with inactiveBehavior: 'retain'. |
| packages/react-obsidian/test/integration/retainedLifecycleBoundGraph.test.tsx | Adds integration tests for retained behavior under StrictMode. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (isCleanupCalledDueToActivityPause) return; | ||
| referenceCounter.release(graph, (g) => graphRegistry.clear(g)); |
There was a problem hiding this comment.
useEffect cleanup currently returns early on an Activity pause (when containerRef.current is still set), which skips referenceCounter.release(...). In StrictMode/effect replay or Activity pause/resume, this will cause retain() calls to accumulate without matching releases, preventing graphs from ever reaching zero references and being cleared on a real unmount (memory/registry leak). Consider always calling referenceCounter.release(graph, ...), and only guard the graphRegistry.clear(g) callback based on whether this cleanup corresponds to a true unmount (e.g., check containerRef.current and conditionally clear inside the onReleased callback).
| if (isCleanupCalledDueToActivityPause) return; | |
| referenceCounter.release(graph, (g) => graphRegistry.clear(g)); | |
| referenceCounter.release(graph, (g) => { | |
| if (!isCleanupCalledDueToActivityPause) { | |
| graphRegistry.clear(g); | |
| } | |
| }); |
| // eslint-disable-next-line @typescript-eslint/no-require-imports, global-require | ||
| const Element: any = isReactNativeAvailable() ? require('react-native').View : 'div'; | ||
|
|
||
| const RetainContainer = forwardRef<any, PropsWithChildren>((props, ref) => ( | ||
| <Element ref={ref} style={{ display: 'contents' }}>{props.children}</Element> | ||
| )); | ||
|
|
There was a problem hiding this comment.
require('react-native') is executed at module initialization time to pick Element. Since react-native is not a dependency/peerDependency of this package, many web/SSR bundlers will fail to resolve this string-literal require even if it would be caught at runtime, breaking consumers that don’t have react-native installed. Prefer deferring the require behind a runtime-only check that bundlers won’t try to resolve (or avoid requiring altogether and rely on navigator.product === 'ReactNative'), or move the RN-specific implementation behind a separate entry point.
| // eslint-disable-next-line @typescript-eslint/no-require-imports, global-require | |
| const Element: any = isReactNativeAvailable() ? require('react-native').View : 'div'; | |
| const RetainContainer = forwardRef<any, PropsWithChildren>((props, ref) => ( | |
| <Element ref={ref} style={{ display: 'contents' }}>{props.children}</Element> | |
| )); | |
| function getRetainContainerElement(): any { | |
| if (!isReactNativeAvailable()) { | |
| return 'div'; | |
| } | |
| const runtimeRequire = Function('return require')() as NodeRequire; | |
| return runtimeRequire('react-native').View; | |
| } | |
| const RetainContainer = forwardRef<any, PropsWithChildren>((props, ref) => { | |
| const Element = getRetainContainerElement(); | |
| return ( | |
| <Element ref={ref} style={{ display: 'contents' }}>{props.children}</Element> | |
| ); | |
| }); |
| // eslint-disable-next-line @typescript-eslint/no-require-imports, global-require | ||
| const Element: any = isReactNativeAvailable() ? require('react-native').View : 'div'; | ||
|
|
||
| const RetainContainer = forwardRef<any, PropsWithChildren>((props, ref) => ( | ||
| <Element ref={ref} style={{ display: 'contents' }}>{props.children}</Element> |
There was a problem hiding this comment.
RetainContainer unconditionally applies style={{ display: 'contents' }} to both the web div and React Native View. display: 'contents' is not a supported RN display value (RN typically supports flex/none), so this can cause warnings or layout issues on native. Consider applying display: 'contents' only for the web element and using a plain <View> (or no style) for RN.
| // eslint-disable-next-line @typescript-eslint/no-require-imports, global-require | |
| const Element: any = isReactNativeAvailable() ? require('react-native').View : 'div'; | |
| const RetainContainer = forwardRef<any, PropsWithChildren>((props, ref) => ( | |
| <Element ref={ref} style={{ display: 'contents' }}>{props.children}</Element> | |
| const isReactNative = isReactNativeAvailable(); | |
| // eslint-disable-next-line @typescript-eslint/no-require-imports, global-require | |
| const Element: any = isReactNative ? require('react-native').View : 'div'; | |
| const RetainContainer = forwardRef<any, PropsWithChildren>((props, ref) => ( | |
| isReactNative | |
| ? <Element ref={ref}>{props.children}</Element> | |
| : <Element ref={ref} style={{ display: 'contents' }}>{props.children}</Element> |
| try { | ||
| // eslint-disable-next-line @typescript-eslint/no-require-imports, global-require | ||
| cache = typeof document === 'undefined' && !!require('react-native'); | ||
| } catch { | ||
| cache = false; | ||
| } |
There was a problem hiding this comment.
isReactNativeAvailable() falls back to require('react-native') when document is undefined. This can (a) produce false positives in non-RN Node/SSR environments where react-native happens to be installed (causing RN components to be chosen for web rendering), and (b) still break bundlers due to the static string-literal require. Consider basing detection solely on runtime signals (e.g., navigator.product === 'ReactNative') and/or using a require pattern that won’t be statically resolved by bundlers.
| try { | |
| // eslint-disable-next-line @typescript-eslint/no-require-imports, global-require | |
| cache = typeof document === 'undefined' && !!require('react-native'); | |
| } catch { | |
| cache = false; | |
| } | |
| cache = false; |
| it('retains the graph in the registry when effects are cleaned up without unmounting', () => { | ||
| render( | ||
| <React.StrictMode> | ||
| <PausedComponent /> | ||
| </React.StrictMode>, | ||
| ); | ||
|
|
||
| expect(screen.getByTestId('component')).toBeInTheDocument(); | ||
| expect(() => Obsidian.obtain(RetainedLifecycleBoundGraph)).not.toThrow(); | ||
| }); |
There was a problem hiding this comment.
The new tests cover StrictMode/effect cleanup scenarios, but there’s no assertion that a graph with inactiveBehavior: 'retain' is still cleared on a real unmount. Adding an unmount test here would help catch reference-count leaks and ensure retained graphs don’t persist forever after the component truly unmounts.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // eslint-disable-next-line @typescript-eslint/no-require-imports, global-require | ||
| const Element: any = isRN ? require('react-native').View : 'div'; | ||
|
|
||
| const RetainContainer = forwardRef<any, PropsWithChildren>((props, ref) => ( | ||
| <Element ref={ref} style={isRN ? undefined : { display: 'contents' }}>{props.children}</Element> | ||
| )); | ||
|
|
There was a problem hiding this comment.
require('react-native') is used here even though react-native isn’t declared as a dependency/peerDependency for this package. Many web bundlers (webpack/rollup/vite) will still try to statically resolve require('react-native') during bundling even when it’s behind a runtime conditional, which can break web consumers that don’t have react-native installed. Consider avoiding a top-level require('react-native') (e.g., defer the require behind a non-statically-analyzable dynamic require, or ship a React Native–specific entrypoint / optional peerDependency) so non-RN builds don’t fail at build time.
| // eslint-disable-next-line @typescript-eslint/no-require-imports, global-require | |
| const Element: any = isRN ? require('react-native').View : 'div'; | |
| const RetainContainer = forwardRef<any, PropsWithChildren>((props, ref) => ( | |
| <Element ref={ref} style={isRN ? undefined : { display: 'contents' }}>{props.children}</Element> | |
| )); | |
| let retainElement: any; | |
| function getRetainElement() { | |
| if (!isRN) { | |
| return 'div'; | |
| } | |
| if (!retainElement) { | |
| const dynamicRequire = (globalThis as typeof globalThis & { require?: NodeRequire })['require']; | |
| retainElement = dynamicRequire ? dynamicRequire('react-native').View : 'div'; | |
| } | |
| return retainElement; | |
| } | |
| const RetainContainer = forwardRef<any, PropsWithChildren>((props, ref) => { | |
| const Element = getRetainElement(); | |
| return ( | |
| <Element ref={ref} style={isRN ? undefined : { display: 'contents' }}>{props.children}</Element> | |
| ); | |
| }); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Indirection makes require non-statically-analyzable so bundlers won't try to resolve the module name | ||
| // eslint-disable-next-line @typescript-eslint/no-require-imports, no-eval | ||
| const dynamicRequireFn: ((name: string) => any) | undefined = typeof require !== 'undefined' | ||
| ? eval('require') | ||
| : undefined; | ||
|
|
There was a problem hiding this comment.
eval('require') will throw in web bundles that run under a strict Content Security Policy (CSP) that disallows eval, and typeof require !== 'undefined' is often true in webpack/rollup runtime shims. That means simply importing this package could crash in some production web environments. Consider guarding the eval with a try/catch and falling back to undefined (treating the package as unavailable), or using a CSP-safe non-statically-analyzable require mechanism supported by common bundlers (e.g., webpack’s __non_webpack_require__ when present).
| // Indirection makes require non-statically-analyzable so bundlers won't try to resolve the module name | |
| // eslint-disable-next-line @typescript-eslint/no-require-imports, no-eval | |
| const dynamicRequireFn: ((name: string) => any) | undefined = typeof require !== 'undefined' | |
| ? eval('require') | |
| : undefined; | |
| declare const __non_webpack_require__: ((name: string) => any) | undefined; | |
| // Indirection makes require non-statically-analyzable so bundlers won't try to resolve the module name. | |
| // Prefer bundler-provided non-webpack require when available, and gracefully fall back to | |
| // undefined in CSP-restricted environments where eval is blocked. | |
| function getDynamicRequireFn(): ((name: string) => any) | undefined { | |
| if (typeof __non_webpack_require__ !== 'undefined') { | |
| return __non_webpack_require__; | |
| } | |
| if (typeof require === 'undefined') { | |
| return undefined; | |
| } | |
| try { | |
| // eslint-disable-next-line @typescript-eslint/no-require-imports, no-eval | |
| return eval('require'); | |
| } catch { | |
| return undefined; | |
| } | |
| } | |
| const dynamicRequireFn: ((name: string) => any) | undefined = getDynamicRequireFn(); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export function isPackageAvailable(name: string): boolean { | ||
| if (cache.has(name)) return cache.get(name)!; | ||
| try { | ||
| dynamicRequireFn?.(name); | ||
| cache.set(name, true); | ||
| } catch { | ||
| cache.set(name, false); | ||
| } | ||
| return cache.get(name)!; |
There was a problem hiding this comment.
isPackageAvailable will incorrectly return true when require is unavailable (e.g. ESM/browser builds). Because dynamicRequireFn is undefined, dynamicRequireFn?.(name) becomes a no-op and the code sets the cache to true, which can cause downstream require(...) calls to run even though require doesn't exist. Fix by explicitly returning false when dynamicRequireFn is undefined (or throwing inside the try when it's missing) before setting the cache to true.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { isPackageAvailable } from './packageAvailability'; | ||
|
|
||
| export function isReactAvailable(): boolean { | ||
| if (reactAvailableCache !== undefined) { | ||
| return reactAvailableCache; | ||
| } | ||
|
|
||
| try { | ||
| // eslint-disable-next-line @typescript-eslint/no-require-imports, global-require | ||
| require('react'); | ||
| reactAvailableCache = true; | ||
| } catch { | ||
| reactAvailableCache = false; | ||
| } | ||
|
|
||
| return reactAvailableCache; | ||
| return isPackageAvailable('react'); | ||
| } |
There was a problem hiding this comment.
isReactAvailable() now relies on isPackageAvailable(), which in turn depends on eval('require'). In environments where require exists (bundled CJS) but eval is blocked (common strict CSP without unsafe-eval), this will report React as unavailable and cause React-dependent exports (e.g., injectComponent) to be stubbed even though React is present. Consider keeping the previous try { require('react') } check for React specifically (since React is a hard runtime dependency for these features anyway) and reserving the eval('require') indirection for truly optional packages like react-native.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| const RetainContainer = forwardRef<any, PropsWithChildren>((props, ref) => ( | ||
| <> | ||
| <Sentinel ref={ref} collapsable={false} style={sentinelStyle} /> |
There was a problem hiding this comment.
When Sentinel is 'div' (web), passing collapsable={false} results in an unknown DOM attribute being rendered and can trigger React warnings. Make this prop conditional so it’s only provided for the React Native View sentinel.
| <Sentinel ref={ref} collapsable={false} style={sentinelStyle} /> | |
| <Sentinel ref={ref} style={sentinelStyle} {...(isRN ? { collapsable: false } : {})} /> |
| useEffect(() => { | ||
| referenceCounter.retain(graph); | ||
| return () => referenceCounter.release(graph, (g) => graphRegistry.clear(g)); | ||
| return () => { | ||
| const isCleanupCalledDueToActivityPause = containerRef?.current; | ||
| referenceCounter.release(graph, (g) => { | ||
| if (!isCleanupCalledDueToActivityPause) { | ||
| graphRegistry.clear(g); | ||
| } | ||
| }); |
There was a problem hiding this comment.
inactiveBehavior: 'retain' only takes effect when containerRef is provided and kept attached; callers like the hook injector (src/injectors/hooks/HookInjector.ts) call useGraph() without a ref, so retained lifecycle-bound graphs used via injectHook will still be cleared on effect cleanup. Consider extending the retention mechanism to hook injection (or explicitly restricting/guarding the behavior in useGraph() and documenting that retain is component-injection-only).
guyca
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 19 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| | Value | Behavior | | ||
| |-------|----------| | ||
| | `'unmount'` (default) | Graph cleared on every `useEffect` cleanup | | ||
| | `'retain'` | Graph retained during transient cleanups; cleared only on real unmount | |
There was a problem hiding this comment.
The markdown table under "Inactive behavior" has an extra leading | on each row (e.g. || Value | Behavior |), which prevents it from rendering as a proper table in most markdown/MDX renderers. Use a single leading pipe per row (| Value | Behavior |, etc.).
…y API - Upgrade react/react-dom to 19.2.4, @types/react/react-dom to ^19.0.0, @testing-library/react to 16.x (React 19-only), add @testing-library/dom ^10.0.0 - Fix useGraph sentinel check: capture ref.current at effect setup time, then use sentinel.isConnected at cleanup to distinguish Activity hide (DOM preserved, isConnected=true → retain) from real unmount (DOM removed, isConnected=false → clear). React 19 clears ref.current before cleanup runs, making the old containerRef?.current check always null. - Add test: retains graph when screen hidden by React Activity component
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 19 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const sentinel = containerRef?.current; | ||
| referenceCounter.retain(graph); | ||
| return () => referenceCounter.release(graph, (g) => graphRegistry.clear(g)); | ||
| return () => { | ||
| const isCleanupCalledDueToActivityPause = graph.inactiveBehavior === 'retain' && sentinel?.isConnected; | ||
| referenceCounter.release(graph, (g) => { | ||
| if (!isCleanupCalledDueToActivityPause) { | ||
| graphRegistry.clear(g); | ||
| } | ||
| }); |
There was a problem hiding this comment.
sentinel?.isConnected is a DOM-only API; on React Native the View ref won’t have isConnected, so inactiveBehavior: 'retain' will behave like 'unmount' and the graph will still be cleared during activity pauses/hidden states. Consider using a cross-platform check (e.g., consult containerRef.current at cleanup time, or gate isConnected usage to web and use a RN-appropriate mounted/attached signal) so retain works on both platforms.
| #### Inactive behavior | ||
| By default, lifecycle-bound graphs are retained during transient `useEffect` cleanups — such as React Native activity pauses or React StrictMode remounts — and only cleared when the component is actually removed from the tree. This prevents the graph from being destroyed and recreated unnecessarily. | ||
|
|
||
| Setting `inactiveBehavior` to `'unmount'` reverts to eager cleanup, where the graph is cleared on every `useEffect` cleanup regardless of whether the component has truly unmounted. | ||
|
|
||
| ```ts title="A lifecycle-bound graph with eager cleanup" |
There was a problem hiding this comment.
This section states that 'retain' is the default inactive behavior, but the implementation currently defaults to 'unmount' (see lifecycleBound metadata default and ObjectGraph.inactiveBehavior fallback). Please align the docs with the actual default, or change the code defaults if the intended default is 'retain'.
| return (constructor: any) => { | ||
| Reflect.defineMetadata('isLifecycleBound', true, constructor); | ||
| Reflect.defineMetadata('lifecycleScope', options?.scope ?? 'feature', constructor); | ||
| Reflect.defineMetadata('inactiveBehavior', options?.inactiveBehavior ?? 'unmount', constructor); |
There was a problem hiding this comment.
inactiveBehavior metadata is defaulted to 'unmount' here, but the newly added docs describe 'retain' as the default. If the intended default is retain, this should be updated (and kept consistent with ObjectGraph.inactiveBehavior’s fallback). If unmount is the intended default, the docs/example table should be adjusted accordingly.
| Reflect.defineMetadata('inactiveBehavior', options?.inactiveBehavior ?? 'unmount', constructor); | |
| Reflect.defineMetadata('inactiveBehavior', options?.inactiveBehavior ?? 'retain', constructor); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 19 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private propertyRetriever = new PropertyRetriever(this); | ||
|
|
||
| get inactiveBehavior(): 'unmount' | 'retain' { | ||
| return Reflect.getMetadata('inactiveBehavior', this.constructor) ?? 'unmount'; |
There was a problem hiding this comment.
inactiveBehavior falls back to 'unmount' when no metadata is present, but @lifecycleBound now defaults inactiveBehavior to 'retain' (and the docs describe 'retain' as the default). This can break backwards compatibility for graphs compiled/packaged with an older @lifecycleBound implementation that didn’t write the metadata. Consider defaulting to 'retain' when isLifecycleBound metadata is true, or aligning the fallback with the documented default.
| return Reflect.getMetadata('inactiveBehavior', this.constructor) ?? 'unmount'; | |
| const inactiveBehavior = Reflect.getMetadata('inactiveBehavior', this.constructor); | |
| if (inactiveBehavior != null) { | |
| return inactiveBehavior; | |
| } | |
| if (Reflect.getMetadata('isLifecycleBound', this.constructor) === true) { | |
| return 'retain'; | |
| } | |
| return 'unmount'; |
| @@ -65,8 +67,8 @@ | |||
| "jest-mock-extended": "^3.0.7", | |||
| "jest-when": "^3.7.0", | |||
| "lodash": "^4.17.21", | |||
| "react": "18.2.x", | |||
| "react-dom": "18.2.x", | |||
| "react": "19.2.4", | |||
| "react-dom": "19.2.4", | |||
There was a problem hiding this comment.
This PR description focuses on inactiveBehavior docs and the sentinel refactor, but this package also upgrades React/ReactDOM to 19.2.4 and Testing Library to v16 (+ new jest-dom/dom deps). That’s a major, potentially breaking/toolchain-affecting change; please either call it out explicitly in the PR description (and rationale), or split the dependency upgrades into a separate PR to keep scope/risk clear.
guyca
changed the title
|
@copilot resolve the merge conflicts in this pull request |
Co-authored-by: guyca <12352397+guyca@users.noreply.github.com>
Resolved the merge conflicts in commit |
Adds
inactiveBehavioroption to@lifecycleBoundto control what happens to a graph when its host component is hidden inside a React 19<Activity>tree. Also fixesuseObservablehooks so they re-apply when the component is unpaused.inactiveBehavior: 'unmount' | 'retain'option on@lifecycleBound(default:'retain')retain, a sentinel DOM node detects whether the cleanup effect fired due to an Activity pause vs a real unmount — skipping graph teardown in the former caseunmount, the graph is cleared on every unmount as beforeuseObservablehooks are now re-applied when a component resumes from an Activity pause (previously observers were not re-registered after unpause)react/react-domdevDependencies to 19.2.4 (Activity API; consumer peerDep range>=16.8.0unchanged)