fix: iframe auth api request loop #37295
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
WalkthroughThe iframe authentication hook is refactored from Changes
Sequence DiagramsequenceDiagram
participant LP as LoginPage
participant UE as useIframe<br/>(useEffectEvent)
participant API as Rocket.Chat<br/>API
participant CB as Callback
LP->>LP: useEffect (on mount/enabled change)
LP->>UE: tryLogin(callback)
activate UE
rect rgb(240, 248, 255)
note right of UE: Build URL with proper<br/>separator & Electron flag
UE->>UE: Construct API URL
end
UE->>API: fetch(apiUrl, {method, credentials})
activate API
API-->>UE: response
deactivate API
alt Success (result.ok)
rect rgb(220, 240, 220)
UE->>UE: Parse response.json()
UE->>UE: loginWithToken(body, callback)
UE->>CB: callback(null, result)
end
else Error
rect rgb(255, 220, 220)
UE->>UE: setIframeLoginUrl(url)
UE->>CB: callback(error, null)
end
end
deactivate UE
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The changes involve heterogeneous modifications across two files: a complex hook rewrite with async logic and callback propagation in Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-7.10.3 #37295 +/- ##
=================================================
Coverage ? 66.22%
=================================================
Files ? 3289
Lines ? 110364
Branches ? 20910
=================================================
Hits ? 73087
Misses ? 34592
Partials ? 2685
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
apps/meteor/client/hooks/iframe/useIframe.ts (6)
40-49: Use URL/URLSearchParams instead of manual separator logic.Safer and clearer than string concatenation; also future‑proof if more params are added.
Apply:
- let url = accountIframeUrl; - let separator = '?'; - if (url.indexOf('?') > -1) { - separator = '&'; - } - - if (navigator.userAgent.indexOf('Electron') > -1) { - url += `${separator}client=electron`; - } + const urlObj = new URL(accountIframeUrl, window.location.origin); + if (navigator.userAgent.includes('Electron')) { + urlObj.searchParams.set('client', 'electron'); + } + const url = urlObj.toString();
18-33: Avoid double dispatch when bothloginTokenandtokenexist.Current logic can invoke both
tokenLoginandiframeLogin. Prefer an exclusive branch and early return.const loginWithToken = useCallback( - (tokenData: string | { loginToken: string } | { token: string }, callback?: (error: Error | null | undefined) => void) => { + (tokenData: string | { loginToken: string } | { token: string }, callback?: (error: Error | null | undefined) => void) => { if (typeof tokenData === 'string') { tokenData = { token: tokenData, }; } - if ('loginToken' in tokenData) { - tokenLogin(tokenData.loginToken, callback); - } - if ('token' in tokenData) { - iframeLogin(tokenData.token, callback); - } + if ('loginToken' in tokenData) { + return tokenLogin(tokenData.loginToken, callback); + } + if ('token' in tokenData) { + return iframeLogin(tokenData.token, callback); + } + callback?.(new Error('Missing token data')); }, [iframeLogin, tokenLogin], );
51-55: Harden network call: normalize method, set Accept header, consider abort/timeout.
- Normalize to an allowlist of methods (GET/POST).
- Add
Accept: application/json.- Consider
AbortControllerto prevent state updates after unmount and to avoid hanging requests.- const result = await fetch(apiUrl, { - method: apiMethod, - headers: undefined, - credentials: 'include', - }); + const method = apiMethod?.toUpperCase() === 'POST' ? 'POST' : 'GET'; + const response = await fetch(apiUrl, { + method, + credentials: 'include', + headers: { Accept: 'application/json' }, + });
57-61: Improve error reporting and renameresulttoresponsefor clarity.
- Use
!response.okonly; include status details in the error.- Avoid confusing naming (
resultis also a param name in the callback type).- if (!result.ok || result.status !== 200) { - setIframeLoginUrl(url); - callback?.(new Error(), null); - return; - } - - const body = await result.json(); + if (!response.ok) { + setIframeLoginUrl(url); + callback?.(new Error(`Iframe auth API failed: ${response.status} ${response.statusText}`), null); + return; + } + + const body = await response.json();Also applies to: 63-64
72-75: Normalize thrown values toError.Ensure the callback always receives an
Errorwith a message for diagnostics.- } catch (error) { - setIframeLoginUrl(url); - callback?.(error instanceof Error ? error : undefined, null); + } catch (error) { + setIframeLoginUrl(url); + const err = error instanceof Error ? error : new Error(String(error)); + callback?.(err, null); }
51-55: Optional: add an AbortController with timeout.Prevents hanging fetch and state updates after unmount. Keep minimal if you prefer, but recommended.
Example (outside the selected lines to illustrate structure):
const controller = new AbortController(); const timer = setTimeout(() => controller.abort(), 10000); try { const response = await fetch(apiUrl, { method, credentials: 'include', headers: { Accept: 'application/json' }, signal: controller.signal }); // ... } finally { clearTimeout(timer); }apps/meteor/client/views/root/MainLayout/LoginPage.tsx (1)
16-23: Optional: guard against double‑invoke in React 18 StrictMode (dev‑only).In dev, effects run twice on mount. A simple ref prevents duplicate API calls.
+import { useEffect, useRef } from 'react'; ... - useEffect(() => { - if (!iframeEnabled) { - return; - } - tryLogin(); - }, [tryLogin, iframeEnabled]); + const attemptedRef = useRef(false); + useEffect(() => { + if (!iframeEnabled || attemptedRef.current) { + return; + } + attemptedRef.current = true; + tryLogin(); + }, [tryLogin, iframeEnabled]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/meteor/client/hooks/iframe/useIframe.ts(3 hunks)apps/meteor/client/views/root/MainLayout/LoginPage.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/client/views/root/MainLayout/LoginPage.tsx (1)
apps/meteor/client/hooks/iframe/useIframe.ts (1)
useIframe(5-84)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: 📦 Meteor Build - coverage
🔇 Additional comments (5)
apps/meteor/client/hooks/iframe/useIframe.ts (3)
35-38: Good guard to prevent unnecessary work when disabled.Short-circuiting on
!enabledis correct and helps avoid accidental loops or spurious requests.
1-1: Nice move touseEffectEventto stop identity churn and request loops.This should stabilize
tryLoginand prevent re‑triggering effects due to changing callback references.Also applies to: 35-35
18-33: No issues found with callback compatibility.All three external usages of
loginWithToken(ResetPasswordPage.tsx:76, useLoginViaQuery.ts:17, ComposerAnonymous.tsx:26) call it with a single token parameter and no callback—which is correct since the callback parameter is optional. The internal use within useIframe.ts (line 26) provides the callback, also valid. The optional callback signature accommodates both patterns without type or behavior drift.apps/meteor/client/views/root/MainLayout/LoginPage.tsx (2)
16-23: Effect correctly initiates login without creating loops.Dependencies are minimal and
useEffectEventkeepstryLoginstable. Looks good.
14-15: DestructuringenabledasiframeEnabledis clear and consistent.Nice clarity improvement at the call site.
yash-rajpal
added
stat: QA assured
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Backports #37021
Summary by CodeRabbit