fix(channels): read checkbox state before the setState updater runs (#5161) - #5279
Conversation
…inyhumansai#5161) The "also delete memory" checkbox in the Telegram and Discord config panels read `event.currentTarget.checked` from inside a functional setState updater: onChange={event => setClearMemoryOnDisconnect(prev => ({ ...prev, [compositeKey]: event.currentTarget.checked, })) } React resets `currentTarget` to null as soon as the handler returns, and a functional updater is not evaluated in the handler — React invokes it later while processing the update queue. The read therefore hit a nulled `currentTarget` and threw `TypeError: Cannot read properties of null (reading 'checked')`. It only failed sometimes because React evaluates an updater eagerly while the fiber has no pending work, which keeps the read inside the handler's synchronous window. Once any other update was already queued — a second toggle, a concurrent render — the eager path is skipped and the updater ran against the nulled event. That matches the low, spread-out event count in Sentry (TAURI-REACT-39) and the `useState` frame in its stack. Read `checked` synchronously in the handler and close the updater over the captured value. Three sites: Telegram's checkbox and both of Discord's (the managed_dm branch and the all-other-modes branch). `ComposioConnectModal` has the same checkbox but passes a plain value, so it was never affected; a sweep of all 35 `currentTarget` reads in `app/src` confirms no other deferred read remains. Also drop the unreachable try/catch in `analyticsInteractions.ts`, which attributed this issue to a disconnected DOM node. `HTMLInputElement`'s `checked` getter works on disconnected and never-mounted nodes and cannot throw, so the guard could never fire and its comment misdiagnosed the bug for the next reader. Regression tests fire three toggles in one batch so the first takes the eager path and the rest defer, then assert the checkbox is still checked and the disconnect call carries `clearMemory: true`. Verified failing before the fix with the exact production error, one case per patched render branch.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 34 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
M3gA-Mind has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
1 participant
Summary
TypeError: Cannot read properties of null (reading 'checked')(SentryTAURI-REACT-39, ~8 events / ~7 users across 4 shortIds).event.currentTarget.checkedfrom inside a functional setState updater, which React invokes after it has already nulledcurrentTarget.checkedsynchronously in the handler and let the updater close over the captured value — three sites (Telegram ×1, Discord ×2).try/catchinanalyticsInteractions.tsthat misattributed this issue to a disconnected DOM node.Problem
The memory checkbox in both channel config panels was wired like this:
React resets
event.currentTargettonullas soon as the handler returns. A functional updater is not evaluated inside the handler — React invokes it later, while processing the update queue during render. The read therefore landed on a nulledcurrentTargetand threw.Why it only failed sometimes (~8 events across ~7 users, rather than on every toggle): React evaluates an updater eagerly inside
dispatchSetStatewhile the fiber has no pending work, as a bail-out optimisation. On that path the read still happens inside the handler's synchronous window and succeeds. As soon as any other update was already queued — a second toggle, a concurrent render — the eager path is skipped, the updater runs at render time, andcurrentTargetis gone. This also explains theuseStateframe in the Sentry stack: the throw happens inside React's update-queue processing, not in the event handler.Note the issue's hint pointed at a null element ref. The actual null is the synthetic event's
currentTarget— there is noref.current.checkedanywhere inapp/src.Solution
Capture the value synchronously, then close the updater over it:
Applied at all three affected sites:
TelegramConfig, and both ofDiscordConfig's (themanaged_dmbranch and the all-other-modes branch). The functional-updater form is retained — it is correct for this keyed map — only the event read moves out of it.Coverage of the bug class. All 35
currentTargetreads inapp/srcwere reviewed; the other 32 are synchronous reads inside their handler and are unaffected.ComposioConnectModalrenders the same checkbox but passes a plain value rather than an updater, so it was never at risk.analyticsInteractions.ts. An earlier attempt at this issue wrappedelement.checkedin atry/catch, commented as guarding a disconnected DOM node whosecheckedaccess "throws". Verified against jsdom thatHTMLInputElement'scheckedgetter returns normally on both disconnected and never-mounted nodes and cannot throw — the catch was unreachable, and its comment misdiagnosed this issue for the next reader. Replaced with a direct return and an accurate note. No behaviour change.Submission Checklist
diff-cover) meet the gate enforced by.github/workflows/ci-lite.yml. Runpnpm test:coverageandpnpm test:rustlocally; PRs below 80% on changed lines will not merge.docs/TEST-COVERAGE-MATRIX.mdreflect this change## Relateddocs/RELEASE-MANUAL-SMOKE.md)Closes #NNNin the## RelatedsectionRegression tests
Three cases, one per patched render branch, in
TelegramConfig.test.tsxandDiscordConfig.test.tsx. Each fires three toggles inside oneactbatch: the first takes React's eager-state path, the rest find pending lanes and defer their updater to render time — precisely the window wherecurrentTargetis null.Each then asserts an odd toggle count leaves the box checked and that disconnect carries
clearMemory: true, so the test proves the deferred updates actually applied rather than merely not throwing.Verified failing before the fix by reverting each site in turn, with the exact production error:
TypeError: Cannot read properties of null (reading 'checked')Impact
Related
app/src/services/analyticsInteractions.ts:103has aconsole.debugthat fires in production on every click of an unlabelled interactive control. Untouched here (out of scope); worth removing separately.AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
Validation Run
pnpm --filter openhuman-app format:check— Prettier clean on all five changed filespnpm typecheck— cleanValidation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
clearMemorydisconnect flag it always did.Parity Contract
analyticsInteractions.controlStatereturns exactly the same values as before.controlState'scheckbox/radio,range,select, andaria-checkedbranches are unchanged; the removedcatchwas unreachable, so no fallback path was lost.Duplicate / Superseded PR Handling