regression: iframe auth api request loop #37021
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
WalkthroughRefactors the iframe login hook to replace a useEffect-driven auto-run with a useEffectEvent-based, externally-invoked tryLogin; URL construction (including Electron flag) and fetch/login flow moved inside tryLogin with revised error handling. LoginPage now consumes tryLogin and enabled and auto-invokes tryLogin when enabled. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant LP as LoginPage
participant UI as useIframe Hook
participant S as Auth Server
participant IF as Iframe
U->>LP: Open Login Page
LP->>UI: call useIframe() -> { iframeLoginUrl, enabled, tryLogin }
alt enabled
LP->>UI: invoke tryLogin()
rect rgb(220,240,255)
UI->>UI: build login URL (append client=electron if needed)
UI->>S: fetch(login URL, headers)
alt success (2xx)
S-->>UI: token payload
UI->>S: loginWithToken(token)
S-->>UI: login result
UI-->>LP: callback(success, body)
else non-ok / error
S-->>UI: error/redirect info or network error
UI->>UI: setIframeLoginUrl(url)
UI-->>LP: callback(error)
end
end
else not enabled
LP->>LP: no iframe attempt
end
note over LP,IF: If `iframeLoginUrl` is set, LP renders <iframe src=...>
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #37021 +/- ##
===========================================
- Coverage 67.34% 67.31% -0.03%
===========================================
Files 3339 3339
Lines 113178 113181 +3
Branches 20535 20532 -3
===========================================
- Hits 76217 76189 -28
- Misses 34355 34386 +31
Partials 2606 2606
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
yash-rajpal
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/client/hooks/iframe/useIframe.ts (1)
18-33: Potential double login and double-callback if both loginToken and token exist.
Both branches run independently; if payload contains both fields, we issue two logins and call the callback twice. Also, when neither field exists, we silently do nothing.Apply this diff:
const loginWithToken = useCallback( (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) { + tokenLogin(tokenData.loginToken, callback); + } else if ('token' in tokenData) { + iframeLogin(tokenData.token, callback); + } else { + callback?.(new Error('Missing token data'), undefined); + } }, [iframeLogin, tokenLogin], );
🧹 Nitpick comments (6)
apps/meteor/client/views/root/MainLayout/LoginPage.tsx (1)
16-23: Guard effect dependencies to avoid accidental re-invocation loops.
If tryLogin’s identity is not stable (or changes across HMR/StrictMode), this effect will re‑fire and may cause repeated API hits. Since tryLogin comes from useEffectEvent and should be stable, consider depending only on iframeEnabled to guarantee a single attempt per enablement.Apply this diff:
-useEffect(() => { - if (!iframeEnabled) { - return; - } - - tryLogin(); -}, [tryLogin, iframeEnabled]); +useEffect(() => { + if (!iframeEnabled) { + return; + } + // tryLogin is stable (useEffectEvent); only react to enablement changes. + tryLogin(); +}, [iframeEnabled]);Please confirm useEffectEvent guarantees a stable function identity in our hooks package. If not, we should keep tryLogin in deps and add an in‑flight guard in the hook.
apps/meteor/client/hooks/iframe/useIframe.ts (5)
57-61: Tighten status check and return a meaningful error.
Using both !result.ok and status !== 200 is redundant and hides context in the error.Apply this diff:
- if (!result.ok || result.status !== 200) { - setIframeLoginUrl(url); - callback?.(new Error(), null); - return; - } + if (!result.ok) { + setIframeLoginUrl(url); + callback?.(new Error(`Iframe auth API failed: ${result.status} ${result.statusText}`), null); + return; + }
50-56: Remove no-op headers assignment and consider AbortController.
headers: undefined is redundant. For resilience, optionally pass an AbortSignal from the caller or keep one here to abort stale attempts.Apply this diff:
- const result = await fetch(apiUrl, { - method: apiMethod, - headers: undefined, - credentials: 'include', - }); + const result = await fetch(apiUrl, { + method: apiMethod, + credentials: 'include', + });
46-48: Harden Electron detection against SSR and future UA changes.
Avoid accessing navigator on the server and prefer includes for clarity.Apply this diff:
- if (navigator.userAgent.indexOf('Electron') > -1) { + if (typeof navigator !== 'undefined' && navigator.userAgent.includes('Electron')) { url += `${separator}client=electron`; }
35-75: Add an in-flight guard to make tryLogin idempotent under rapid re-invocations.
Prevents overlapping fetches if multiple components call tryLogin or settings toggle quickly.Apply this diff (plus one new import):
-import { useCallback, useState } from 'react'; +import { useCallback, useRef, useState } from 'react'; @@ -const tryLogin = useEffectEvent(async (callback?: (error: Error | null | undefined, result: unknown) => void) => { +const inFlightRef = useRef(false); +const tryLogin = useEffectEvent(async (callback?: (error: Error | null | undefined, result: unknown) => void) => { if (!enabled) { return; } + if (inFlightRef.current) { + return; + } + inFlightRef.current = true; @@ - } catch (error) { + } catch (error) { setIframeLoginUrl(url); callback?.(error instanceof Error ? error : undefined, null); - } + } finally { + inFlightRef.current = false; + } });
16-17: Enablement predicate looks good. Consider stricter string checks if whitespace is possible.
Minor: trim strings if settings may contain accidental whitespace.
📜 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(2 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-83)
🔇 Additional comments (5)
apps/meteor/client/views/root/MainLayout/LoginPage.tsx (2)
5-5: Importing useEffect is appropriate for driving the event-based login.
14-15: Good: consume new tryLogin/enabled API and alias enabled → iframeEnabled.
This keeps call‑site semantics clear and avoids naming collisions.apps/meteor/client/hooks/iframe/useIframe.ts (3)
1-1: Adopting useEffectEvent for tryLogin is the right direction — verified.
Scanned repo: apps/meteor/client/views/root/MainLayout/LoginPage.tsx and apps/meteor/client/hooks/iframe/useIframeLoginListener.ts both explicitly invoke tryLogin (useEffect, message handlers, and passed to unstoreLoginToken); no consumer relies on automatic invocation.
8-12: Settings keys validated — casing matches repository settings.
useSetting keys in useIframe.ts (Accounts_iframe_enabled, Accounts_iframe_url, Accounts_Iframe_api_url, Accounts_Iframe_api_method) match the repository i18n/settings entries. (fossies.org)
63-70: Ignore type import suggestion Meteor.Error and Meteor.TypedError come from the ambient Meteor definitions and are globally available—no import or widening is needed.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/client/hooks/iframe/useIframe.ts (1)
18-33: Ensure callback is always invoked; avoid param mutation.If the response lacks both token shapes,
loginWithTokencurrently does nothing and never calls the callback, leaving callers hanging and potentially re-triggering flows.Apply:
- const loginWithToken = useCallback( - (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); - } - }, - [iframeLogin, tokenLogin], - ); + const loginWithToken = useCallback( + (tokenData: string | { loginToken: string } | { token: string }, callback?: (error: Error | null | undefined) => void) => { + const payload = typeof tokenData === 'string' ? { token: tokenData } : tokenData; + if ('loginToken' in payload) { + tokenLogin(payload.loginToken, callback); + return; + } + if ('token' in payload) { + iframeLogin(payload.token, callback); + return; + } + callback?.(new Error('Invalid token payload')); + }, + [iframeLogin, tokenLogin], + );
🧹 Nitpick comments (4)
apps/meteor/client/hooks/iframe/useIframe.ts (4)
40-48: Prefer URL API for query assembly.Manual “? / &” logic is brittle. The URL API is safer for existing queries and hashes.
Example:
- let url = accountIframeUrl; - let separator = '?'; - if (url.indexOf('?') > -1) { - separator = '&'; - } - if (navigator.userAgent.indexOf('Electron') > -1) { - url += `${separator}client=electron`; - } + const u = new URL(accountIframeUrl, window.location.origin); + if (navigator.userAgent.indexOf('Electron') > -1) { + u.searchParams.set('client', 'electron'); + } + const url = u.toString();
51-55: Tighten fetch options.
- Remove
headers: undefined.- Send
Accept: application/json.- Ensure
credentials: 'include'is intentional for cross-origin.
57-61: Simplify error check and surface status in the error.
!result.oksuffices; include status/statusText for diagnostics.Apply (also included in a larger diff above):
- if (!result.ok || result.status !== 200) { - setIframeLoginUrl(url); - callback?.(new Error(), null); - return; - } + if (!result.ok) { + const err = new Error(`Iframe auth API ${result.status} ${result.statusText || ''}`.trim()); + setIframeLoginUrl(url); + callback?.(err, null); + return; + }
64-71: Drop unnecessary async on callback.The inner callback doesn’t
await; removeasyncto avoid creating an unused promise.- loginWithToken(body, async (error: Meteor.Error | Meteor.TypedError | Error | null | undefined) => { + loginWithToken(body, (error: Meteor.Error | Meteor.TypedError | Error | null | undefined) => {
📜 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 (1)
apps/meteor/client/hooks/iframe/useIframe.ts(2 hunks)
🔇 Additional comments (2)
apps/meteor/client/hooks/iframe/useIframe.ts (2)
63-71: Good fix: parse body once; no double-consume.The prior “body used already” bug is resolved by storing
const body = await result.json()and reusing it.
1-1: Confirm useEffectEvent provides fresh-state semantics.yarn.lock shows @rocket.chat/fuselage-hooks@^0.37.0 and many files import useEffectEvent, but the automated search didn’t locate the package export — verify in node_modules/@rocket.chat/fuselage-hooks that useEffectEvent is implemented to read fresh state (does not capture stale closures); if not, upgrade or pin to a version that does.
yash-rajpal
added
stat: ready to merge
|
/backport 7.10.3 |
|
Sorry, I couldn't do that backport because of conflicts. Could you please solve them? you can do so by running the following commands: after that just run |
Proposed changes (including videos or screenshots)
The iframe auth API is being triggered hundreds of times by the client due to a dependency loop and a race condition.
Issue(s)
Steps to test or reproduce
Further comments
CORE-1390
Summary by CodeRabbit
New Features
Bug Fixes
Refactor