Convert handler-only pendingFormValues from useState to useRef on Alternative Login Identifier page - #10586
Open
dannguyen24 wants to merge 4 commits into
Open
Convert handler-only pendingFormValues from useState to useRef on Alternative Login Identifier page#10586dannguyen24 wants to merge 4 commits into
dannguyen24 wants to merge 4 commits into
Conversation
React Doctor's rerender-state-only-in-handlers rule flags pendingFormValues in alternative-login-identifier-page.tsx: it is written in processFormSubmission and read in handleConsentedSubmit, but never in the component's return. Each setPendingFormValues therefore schedules a re-render with no visible change. Move it to useRef so the value still persists across the confirmation-modal round-trip while setting it no longer forces a render. The re-render the modal relies on still comes from setShowConfirmationModal(true), so behavior is identical (pure performance change). connector (also flagged) is left as useState: it is a false positive, read in the useEffect dependency array that drives initializeForm()/initialFormValues (which is rendered). Converting it would stop the effect from firing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The lockfile changes were unintended environment noise (stripped libc fields, changed dependency hash) unrelated to the useRef refactor. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThe changes replace handler-only React state with refs across account, consent, recovery, association, and session components, while preserving pending submissions, cached lookups, initialization checks, form values, and modal targets. ChangesHandler-Only State Refactor
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
features/admin.alternative-login-identifier.v1/pages/alternative-login-identifier-page.tsx (1)
113-116: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueType the ref as nullable.
pendingFormValuesRefis initialized withnull, so its explicit type should includenullrather than narrowing it to the form values only.Proposed fix
- const pendingFormValuesRef: MutableRefObject<AlternativeLoginIdentifierFormInterface> = - useRef<AlternativeLoginIdentifierFormInterface>(null); + const pendingFormValuesRef: MutableRefObject<AlternativeLoginIdentifierFormInterface | null> = + useRef<AlternativeLoginIdentifierFormInterface | null>(null); const handleConsentedSubmit = (): void => { - if (!pendingFormValuesRef.current) { + const pendingFormValues: AlternativeLoginIdentifierFormInterface | null = + pendingFormValuesRef.current; + + if (!pendingFormValues) { return; } - processFormSubmission(pendingFormValuesRef.current, true); + processFormSubmission(pendingFormValues, true);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@features/admin.alternative-login-identifier.v1/pages/alternative-login-identifier-page.tsx` around lines 113 - 116, Update the explicit type of pendingFormValuesRef to allow null, matching its useRef initialization while preserving the existing AlternativeLoginIdentifierFormInterface value type.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@features/admin.alternative-login-identifier.v1/pages/alternative-login-identifier-page.tsx`:
- Around line 113-116: Update the explicit type of pendingFormValuesRef to allow
null, matching its useRef initialization while preserving the existing
AlternativeLoginIdentifierFormInterface value type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: fcd04961-0479-464b-8baf-fd02ff1ebda5
📒 Files selected for processing (1)
features/admin.alternative-login-identifier.v1/pages/alternative-login-identifier-page.tsx
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #10586 +/- ##
==========================================
+ Coverage 72.71% 73.46% +0.75%
==========================================
Files 469 471 +2
Lines 70846 71454 +608
Branches 484 469 -15
==========================================
+ Hits 51513 52492 +979
+ Misses 19037 18687 -350
+ Partials 296 275 -21 🚀 New features to boost your workflow:
|
Addresses react-doctor rerender-state-only-in-handlers for 6 My Account
components where the value is set and read only inside handlers (or a
run-once mount effect guard) and never in render:
- security-questions-recovery: isInit
- consents: purposeDetailModels
- federated-associations: id
- linked-accounts-edit: userName
- linked-accounts-list: userID
- user-sessions: editingUserSession
Skipped (unsafe to convert) in this area:
- backup-code-authenticator: isMFAConfigured (read in effect body; ref
would change stale-closure semantics)
- dashboard: previousViewportWasMobile (used in an effect dependency array)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/myaccount/src/components/linked-accounts/linked-accounts-edit.tsx`:
- Around line 62-63: Keep handler-only refs nullable in linked-accounts-edit.tsx
at lines 62-63, and initialize userNameRef with an empty string or validate it
before constructing getFormValues(). In linked-accounts-list.tsx lines 57-58,
narrow userIDRef before passing it to handleLinkedAccountRemove(); in
federated-associations.tsx lines 72-73, narrow idRef before passing it to
removeFederatedAssociation().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6d1736eb-c1f1-4ec2-bc56-475f83e9971a
📒 Files selected for processing (6)
apps/myaccount/src/components/account-recovery/options/security-questions-recovery.tsxapps/myaccount/src/components/consents/consents.tsxapps/myaccount/src/components/federated-associations/federated-associations.tsxapps/myaccount/src/components/linked-accounts/linked-accounts-edit.tsxapps/myaccount/src/components/linked-accounts/linked-accounts-list.tsxapps/myaccount/src/components/user-sessions/user-sessions.tsx
Comment on lines
+62
to
+63
| // Captures the latest typed username; read only on submit, never rendered, so a ref avoids per-keystroke re-renders. | ||
| const userNameRef: MutableRefObject<string> = useRef<string>(undefined); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -HI 'tsconfig*.json' -t f -x rg -n '"strictNullChecks"|"strict"' {}
rg -n 'useRef<string>\((null|undefined)\)' apps/myaccount/src/componentsRepository: wso2/identity-apps
Length of output: 2909
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
wc -l apps/myaccount/src/components/linked-accounts/linked-accounts-edit.tsx apps/myaccount/src/components/linked-accounts/linked-accounts-list.tsx apps/myaccount/src/components/federated-associations/federated-associations.tsx
echo "== edit relevant =="
cat -n apps/myaccount/src/components/linked-accounts/linked-accounts-edit.tsx | sed -n '1,130p'
echo "== list relevant =="
cat -n apps/myaccount/src/components/linked-accounts/linked-accounts-list.tsx | sed -n '1,130p'
echo "== federated relevant =="
cat -n apps/myaccount/src/components/federated-associations/federated-associations.tsx | sed -n '1,260p'
echo "== references to values =="
rg -n 'userNameRef|userIDRef|idRef|LinkedAccount|onLinkedAccountRemove|removeFederatedAssociation' apps/myaccount/src/components/linked-accounts apps/myaccount/src/components/federated-associationsRepository: wso2/identity-apps
Length of output: 33976
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== tsconfig files relevant to app =="
for f in package.json apps/myaccount/package.json apps/myaccount/tsconfig.json apps/myaccount/tsconfig.app.json apps/myaccount/tsconfig.dev.json tsconfig.base.json tsconfig.json; do
if [ -f "$f" ]; then
echo "--- $f ---"
sed -n '/"compilerOptions"/,/^[[:space:]]*}/p' "$f" 2>/dev/null | head -80
fi
done
echo "== edit remaining lines =="
cat -n apps/myaccount/src/components/linked-accounts/linked-accounts-edit.tsx | sed -n '130,175p'
echo "== federated remaining relevant lines =="
cat -n apps/myaccount/src/components/federated-associations/federated-associations.tsx | sed -n '260,295p'Repository: wso2/identity-apps
Length of output: 7201
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== vite config files =="
fd -HI 'vite.config.*' . -t f -x sh -c 'echo "--- $1 ---"; sed -n "1,220p" "$1"' sh {}
echo "== tsconfig inheritance =="
for f in apps/myaccount/tsconfig.json tsconfig.base.json; do
if [ -f "$f" ]; then
echo "--- $f ---"
cat "$f"
fi
done
echo "== exact compiler options in apps/myaccount/tsconfig.json =="
python3 - <<'PY'
import json
from pathlib import Path
p=Path("apps/myaccount/tsconfig.json")
data=json.loads(p.read_text())
print(json.dumps(data, indent=2))
PY
echo "== compile-time availability without installing =="
node - <<'JS'
try {
const ts = require("typescript");
console.log("typescript available", ts.version);
} catch (e) {
console.log("typescript not available", e.message);
}
JSRepository: wso2/identity-apps
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== root/react tsconfig null-related options =="
for f in tsconfig.base.react.json tsconfig.base.json apps/myaccount/tsconfig.json; do
if [ -f "$f" ]; then
echo "--- $f ---"
rg -n '"strict"|"strictNullChecks"|"strictPropertyInitialization"|"exactOptionalPropertyTypes"|"paths"|"extends"' "$f" || true
fi
done
echo "== all tsconfig null-related options =="
rg -n --glob 'tsconfig*.json' '"strict"|"strictNullChecks"|"strictPropertyInitialization"|"exactOptionalPropertyTypes"' .Repository: wso2/identity-apps
Length of output: 454
Keep handler-only ref contents nullable.
userIDRef and idRef are typed as non-nullable strings but are initialized/cleared with null; narrow/refine before passing them into handleLinkedAccountRemove() and removeFederatedAssociation(). For userNameRef, initialize with "" or validate the missing value before building getFormValues() so submit cannot receive a non-string username.
📍 Affects 3 files
apps/myaccount/src/components/linked-accounts/linked-accounts-edit.tsx#L62-L63(this comment)apps/myaccount/src/components/linked-accounts/linked-accounts-list.tsx#L57-L58apps/myaccount/src/components/federated-associations/federated-associations.tsx#L72-L73
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/myaccount/src/components/linked-accounts/linked-accounts-edit.tsx`
around lines 62 - 63, Keep handler-only refs nullable in
linked-accounts-edit.tsx at lines 62-63, and initialize userNameRef with an
empty string or validate it before constructing getFormValues(). In
linked-accounts-list.tsx lines 57-58, narrow userIDRef before passing it to
handleLinkedAccountRemove(); in federated-associations.tsx lines 72-73, narrow
idRef before passing it to removeFederatedAssociation().
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.
What does this PR do?
Converts the
pendingFormValuesvalue on the Alternative Login Identifier page fromuseStatetouseRef. This value holds the form data that is temporarily stashed while the user is shown the uniqueness-scope confirmation modal. It is only ever written and read inside event handlers (processFormSubmissionandhandleConsentedSubmit) and is never referenced in the component's render output, so backing it with a ref avoids an unnecessary re-render on every update while preserving identical behavior.Why was this PR needed?
The React Doctor linter flagged this location under the
rerender-state-only-in-handlersrule: "Each update topendingFormValuesredraws your component for nothing because this useState is set but never shown on screen." Investigation ofalternative-login-identifier-page.tsxconfirmed the value is set (setPendingFormValues) only when opening the confirmation modal and read only when the user consents — both purely inside handlers, never in the JSX.useRefis the correct primitive for a mutable value that does not drive rendering, so it is swapped in with no functional change.What are the relevant issue numbers?
Closes wso2/product-is#27957
Screenshots / Recordings (if applicable)
Not applicable — this is an internal state-management refactor with no visual or behavioral change. The confirmation-modal consent flow behaves identically.
Does this PR meet the acceptance criteria?
MutableRefObject<...>annotation, typeduseRefgeneric)