Avoid effect-driven PR dialog resets#2975
Draft
cursor[bot] wants to merge 1 commit into
Draft
Conversation
Co-authored-by: Julius Marminge <juliusmarminge@users.noreply.github.com>
Contributor
|
🚀 Expo continuous deployment is ready!
|
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
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.
What Changed
PullRequestThreadDialoginto a keyed outer wrapper plus inner content soinitialReference/open changes reset local dialog state through component ownership instead of a synchronoussetStateeffect.useFocusReferenceInputWhenOpen).Why
React Doctor flagged the dialog reset effect as
set-state-in-effect, prop-state synchronization, cascading state updates, derived state, and reset-all-state-on-prop-change. Keying the inner dialog content follows React's guidance for resetting state on prop changes and avoids the stale intermediate render caused by resetting state after commit.React Doctor after scan: web report improved from 133 errors / 1066 warnings / 1199 total diagnostics to 130 errors / 1063 warnings / 1193 total diagnostics. The dialog reset diagnostics are gone; remaining dialog findings are pre-existing compiler/manual memoization warnings outside this reset path.
UI Changes
No user-facing UI design change. Interaction behavior was verified with React Scan enabled:
react_scan_pr_dialog_before_v2.mp4react_scan_pr_dialog_after_v2.mp4Both recordings use the same temporary diagnostics surface to swap
initialReference123 -> 456 -> 123 while the real dialog remains open. React Scan purple render highlights are visible during the swaps.Checklist
Validation:
bunx --bun react-doctor@latest apps/web --offline --fail-on none --jsonvp check(run with Node 24.16.0 on PATH)vp run typecheck(run with Node 24.16.0 on PATH)Note
Avoid effect-driven state resets in
PullRequestThreadDialogby using key-based remountingPullRequestThreadDialoginto a thin wrapper and a newPullRequestThreadDialogContentchild in PullRequestThreadDialog.tsxstateResetKeyfromopenandinitialReferenceand passes it askeyto the content component, so React remounts the child instead of running a manual state-resetuseEffectuseFocusReferenceInputWhenOpenhook that usesrequestAnimationFrameinstead of a raw effectMacroscope summarized 7544d1c.