Akmal / fix: enhance demo account handling in AuthWrapper and CallbackPage#661
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Manifest Filespackage-lock.json
|
|
Claude finished @akmal-deriv's task in 2m 9s —— View job PR Review: fix: enhance demo account handling in AuthWrapper and CallbackPageTasks:
SummaryThis PR improves demo account selection during authentication by introducing a shared High Priority1. loginid.startsWith('DEM') ||
loginid.startsWith('DOT')Neither The only virtual account prefixes confirmed in the codebase are 2. const wantsDemo =
accountParam.toLowerCase() === 'demo' || defaultActiveAccount?.loginid?.startsWith('VR');The fallback check only uses Medium Priority3. This function is purely about account identity/auth logic, not analytics. It is imported by 4. No unit tests for The function has branching logic and edge cases (empty string, falsy values, overlapping prefixes like it('identifies VRTC as demo', () => expect(isDemoAccount('VRTC1234567')).toBe(true));
it('identifies VRW as demo', () => expect(isDemoAccount('VRW1000001')).toBe(true));
it('identifies real account as non-demo', () => expect(isDemoAccount('CR1234567')).toBe(false));
it('handles empty string', () => expect(isDemoAccount('')).toBe(false));5. // Fallback to first account from authorize response
if (!targetAccount) {
const firstId = authorize?.account_list[0]?.loginid;
targetAccount = loginInfo.find(account => account.loginid === firstId);
}If the user explicitly requested a demo account but none exists in Low Priority6. Reading URL params inside a utility function is a hidden side effect. The 7. The move from after the API block to before it ( Pre-existing issue (not introduced by this PR, but worth flagging)
const { is_tmb_enabled = false } = useTMB();This call is inside Quick-reference checklist
|
|
Claude finished @akmal-deriv's task in 1m 45s —— View job PR Review: fix: enhance demo account handling in AuthWrapper and CallbackPageTasks:
SummaryThis PR introduces High Priority1. loginid.startsWith('DEM') ||
loginid.startsWith('DOT')Searching the entire codebase, every other demo-account check uses
Neither 2. Three separate demo-detection paths remain after this PR — The whole point of introducing // callback-page.tsx:29 — still uses startsWith('VR'), not isDemoAccount
if (tokens.acct1?.startsWith('VR') || currency === 'demo') return 'demo';
// AuthWrapper.tsx:84 — falls back to startsWith('VR'), not isDemoAccount
const wantsDemo =
accountParam.toLowerCase() === 'demo' || defaultActiveAccount?.loginid?.startsWith('VR');If Medium Priority3. const { is_tmb_enabled = false } = useTMB();This is called inside 4. The function is about account identity, not analytics. It is already imported by 5. No unit tests for The function has branching logic (multiple prefixes), is used in two critical auth flows, and has an edge case for empty string. Tests should cover at minimum: it('identifies VRTC as demo', () => expect(isDemoAccount('VRTC1234567')).toBe(true));
it('identifies VRW as demo', () => expect(isDemoAccount('VRW1000001')).toBe(true));
it('identifies CR as non-demo', () => expect(isDemoAccount('CR1234567')).toBe(false));
it('handles empty string', () => expect(isDemoAccount('')).toBe(false));6. Silent fallback to real account when demo was explicitly requested — if (!targetAccount) {
const firstId = authorize?.account_list[0]?.loginid;
targetAccount = loginInfo.find(account => account.loginid === firstId);
}If the user had Low Priority7. Moving Quick-reference checklist
|
|
Claude finished @akmal-deriv's task in 2m 6s —— View job PR Review: fix: enhance demo account handling in AuthWrapper and CallbackPageTasks:
SummaryThis PR introduces High Priority1. loginid.startsWith('DEM') ||
loginid.startsWith('DOT')A search across the entire codebase finds zero other references to
These entries need explicit confirmation against Deriv's account system documentation, or should be removed until confirmed. 2. The stated purpose of // callback-page.tsx:29 — unchanged, still uses startsWith('VR')
if (tokens.acct1?.startsWith('VR') || currency === 'demo') return 'demo';
// AuthWrapper.tsx:84 — fallback still uses startsWith('VR'), not isDemoAccount
const wantsDemo =
accountParam.toLowerCase() === 'demo' || defaultActiveAccount?.loginid?.startsWith('VR');If Medium Priority3. This function is about account identity, not analytics. It is already imported by It belongs alongside 4. No unit tests for The function has branching logic (4 prefix checks), is called from two critical auth flows, and handles an edge case (empty string → it('identifies VRTC as demo', () => expect(isDemoAccount('VRTC1234567')).toBe(true));
it('identifies VRW as demo', () => expect(isDemoAccount('VRW1000001')).toBe(true));
it('identifies CR as non-demo', () => expect(isDemoAccount('CR1234567')).toBe(false));
it('handles empty string', () => expect(isDemoAccount('')).toBe(false));5. Silent fallback to real account when demo was explicitly requested — if (!targetAccount) {
const firstId = authorize?.account_list[0]?.loginid;
targetAccount = loginInfo.find(account => account.loginid === firstId);
}If 6. Pre-existing: const { is_tmb_enabled = false } = useTMB();This is called inside Low Priority7. Reading URL params inside a utility function called deep in the auth flow is a hidden side effect. By the time this line runs, 8. Moving Quick-reference checklist
|
|
Claude finished @akmal-deriv's task in 1m 50s —— View job PR Review: fix: enhance demo account handling in AuthWrapper and CallbackPageTasks:
SummaryThis iteration resolves all of the high-priority findings from previous reviews. Must Fix (Blocks Merge)
// Inside onSignInSuccess, an async function passed as a prop — NOT a component
const { is_tmb_enabled = false } = useTMB();This violates the Rules of Hooks. Hooks must be called at the top level of a React component or custom hook. Calling one inside an async callback silently produces stale/incorrect values in React strict mode, and will generate warnings in dev. The fix is to hoist the hook to the const CallbackPage = () => {
const { is_tmb_enabled = false } = useTMB(); // ← move here
return (
<Callback
onSignInSuccess={async (tokens, rawState) => {
// ... use is_tmb_enabled from closure ...
}}
/>
);
};This is pre-existing code, but Good to HaveMissing test: The five existing tests are exactly the ones recommended in previous reviews — good. One additional case worth adding: it('handles null-ish via the guard', () => {
// TypeScript enforces string, but guard handles falsy at runtime
expect(isDemoAccount('')).toBe(false); // already tested
});Not a blocker, just a suggestion. Pre-existing issue, not introduced here (track separately)
localStorage.setItem('callback_token', authorize.toString());
What was addressed from previous reviews ✅
Quick-reference checklist
|
|
Preview Link: https://fix-reset-to-demo.bot-65f.pages.dev
|
3 participants
Uh oh!
There was an error while loading. Please reload this page.