fix: review follow-ups round 2 + issue triage (#247-#256) - #259
Merged
Conversation
- batch: guard the Error.cause assignment so a frozen/non-extensible callback error no longer throws a TypeError from finally that would replace the in-flight exception (the #212 masking all over again). - batch: when the flush error cannot be attached (callback threw a non-Error, the Error already has a cause, or the object is frozen), console.warn instead of silently dropping the reaction's stack. - observe: reviving an unobserved reaction now resets everRan, so a failing first run after revival takes the firstRun auto-detach path instead of restoring an empty snapshot and becoming a live reaction with zero dependencies (interaction regression between #215 and #233). - runAsReaction: drop the per-rerun cleaners.slice() (releaseReaction never mutates the old array in place, so the reference is a valid snapshot) and the unreachable else-release branch. Co-Authored-By: Claude Code <noreply@anthropic.com>
view-wrapped class components silently lost reactivity after any componentWillUnmount that was not a real unmount — StrictMode's simulated remount and Suspense/Offscreen hide both null the _reactiveRender reaction, and nothing ever recreated it. Recreate in componentDidMount (the remount path replays cDM without re-rendering) and in render as a fallback; also treat an unobserved (first-run auto-detached) reaction as dead. useObserver gains the same unobserved-reaction guard as defense in depth — empirically React 19 remounts mount-suspended fibers (no hook state to preserve) and committed re-suspensions are healed by the subscribe cleanup/recreate path, but a non-null dead reaction must never be reused for tracking. Co-Authored-By: Claude Code <noreply@anthropic.com>
The committed-flag + queueMicrotask destroy only cancels StrictMode's same-flush re-setup. <Activity>/Offscreen hide→reveal spans separate commits, so the microtask destroys the container while the tree is merely hidden and the useRef-preserved ADM never recreates it — after reveal, children resolve against a null container. Rebuild the container in place during render (Activity reveal re-renders the tree) and re-arm the FinalizationRegistry backstop for the rebuilt container. Trade-off, noted in code: service state is lost while hidden. Also: - guard queueMicrotask with a Promise.resolve().then fallback for old RN JSC/Hermes environments (same targets the FinalizationRegistry and WeakRef fallbacks exist for), - drop the post-destroy registry re-register (provable no-op), - drop the dead `timmer` ADM field. Co-Authored-By: Claude Code <noreply@anthropic.com>
- batch: guard against self-referential cause when the callback and a reaction throw the identical Error instance (E.cause = E loops naive cause-chain walkers); fall through to the warn path instead - batch: isolate console.warn itself so throwing warn shims (strict console harnesses) cannot replace the in-flight callback exception - observe(revive): preserve a revived reaction's custom scheduler and debugger instead of silently overwriting them with global defaults - reaction-runner: drop the redundant null encoding on prevCleaners (the restore branch is unreachable on firstRun) Co-Authored-By: Claude Code <noreply@anthropic.com>
- deferToMicrotask: add a setTimeout final fallback so teardown still happens in legacy JS engines without Promise; document that setImmediate-style Promise polyfills degrade the timing guarantee - extract createContainer so the hidden-tree rebuild no longer constructs a throwaway pseudo-ADM wrapper Co-Authored-By: Claude Code <noreply@anthropic.com>
…lifecycles
- view class components no longer create/run the reaction before the
first commit: a discarded render pass (sibling suspends, concurrent
abort) never registers deps, so the reaction cannot leak permanently
in the store and forceUpdate a dead instance. Class components cannot
use the useObserver FinalizationRegistry backstop (the leaked
subgraph store->reaction->instance is self-sustaining), so tracking
starts in componentDidMount with one forced re-render to collect
deps (synchronous before paint, invisible to the UI).
- arrow-function class fields (componentDidMount = () => {...}) are
own instance properties that shadow the wrapper's prototype
lifecycle methods: React invoked only the field, silently skipping
reaction revival on StrictMode/Suspense remounts and reaction
cleanup on unmount. After super() the fields are initialized, so the
wrapper now rebinds them in place to run both the user's field and
the wrapper logic.
Co-Authored-By: Claude Code <noreply@anthropic.com>
…s-reactive # Conflicts: # packages/observer/src/internals/reaction-runner.ts # packages/react/src/domain/bind.tsx # packages/react/src/view.tsx
- #247: pre-register the (instance, key) dependency through the has trap before computing, so a throwing getter no longer leaves outer observers with zero dependencies and permanently silent - #248: invalidate the cache synchronously on the trigger path (the reaction's debugger hook, write-ops only) so mid-batch reads never see stale values; reuse the inner reaction across recomputations so a notify queued mid-batch is not dropped - #255: cleanupAllMemos notifies outer observers (batched, single flush); Service.destroy passes { notify: false } to stay silent Co-Authored-By: Claude Code <noreply@anthropic.com>
Detached calls (this null/undefined/primitive, e.g. arr.map(service.save) or destructured methods) used to throw TypeError: Invalid value used as weak map key after the per-instance WeakMap migration. They now share one module-level sentinel state, matching the pre-WeakMap closure semantics. Co-Authored-By: Claude Code <noreply@anthropic.com>
) Reading a frozen (non-configurable + non-writable) own array mutator returned the batch-wrapped function before the invariant check ran, throwing TypeError. The wrap now happens after the invariant check in the base handler; the shadow handler checks the descriptor only on the narrow would-wrap path so ordinary reads pay nothing extra. Co-Authored-By: Claude Code <noreply@anthropic.com>
Deep mode returns proxy keys (keys === values per #192), shadow mode returns raw members; raw() unwraps iterator-returned proxy keys. Contract tests pin both behaviors and the README documents the raw-collection interop caveat plus the raw() escape hatch. Co-Authored-By: Claude Code <noreply@anthropic.com>
, #253) The two-function form executed the effect inside the tracked callback, so observables read by the effect leaked into the dependency set and spuriously re-fired it. The effect now runs under an unobserved shield reaction (public API only), matching MobX reaction semantics; the fireImmediately first run is covered too. The single-function form now warns in development when the silently dropped lazy option is passed. Co-Authored-By: Claude Code <noreply@anthropic.com>
The constructor early-returned under enableStaticRendering, skipping the lifecycle-field rebinding; with an arrow-field componentDidMount the wrapper's commit logic was then shadowed forever and the component never became reactive after the flag was turned off. Rebinding creates no reaction and is SSR-safe, so it now always runs. Also adds the pending changesets for the earlier fixes on this branch. Co-Authored-By: Claude Code <noreply@anthropic.com>
This was referenced Sep 1, 2026
…252) Escaped service references becoming dead after unmount is intended — the container is private to the bound subtree by design. Co-Authored-By: Claude Code <noreply@anthropic.com>
ximing
changed the title
ximing
added a commit
that referenced
this pull request
Sep 4, 2026
Keep the later review-round implementations; retain master's batch self-cause guard from #259.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
背景
两部分内容：(1) #257 合入后追加的一轮 review 修复（当时在原分支上提交推送，晚于 squash 合并，未进入 master）；(2) 10 个 open issue 的 triage 与修复。全部经过验证 + TDD（先红后绿）。
Review 修复（round 2）
observer
batch: 回调与 reaction 抛出同一 Error 实例时禁止自引用cause；console.warn自身加隔离（防 jest-fail-on-console 类环境重新引入 observer: batch() 的 flush 错误会吞掉回调自身的异常 #212 掩蔽）observe复活路径：不再用全局默认覆盖复活 reaction 的自定义 scheduler/debuggerreact
componentDidMount = () => {}实例字段遮蔽包装器原型方法导致的静默失活/泄漏bindServices:deferToMicrotask增加 setTimeout 最终兜底；提取createContainerIssue triage（#247–#256，全部处理并关闭）
验证
🤖 Generated with Claude Code