Move to loginRedirect from loginPopup, and update e2e tests#293
Move to loginRedirect from loginPopup, and update e2e tests#293devksingh4 merged 7 commits intomainfrom
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughRefactors authentication flows from MSAL popup-based to redirect-based, adding a returnPath parameter to token acquisition. Threads returnPath through AuthActionButton and various buttons (membership, netid sync, store) and adds URL-based initialization for store item variant/quantity. Adds an auth-redirect runtime page that handles MSAL redirect results. Introduces or updates multiple Playwright e2e tests (home, identity sync, membership, resources, navigation) and test IDs in components/layouts. Adds calendar test IDs, membership perks test ID, and updates GitHub Actions to shard e2e runs and merge HTML reports. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Deploying with Cloudflare Pages
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/AuthActionButton.tsx (1)
62-73:⚠️ Potential issue | 🟡 MinorUnhandled error from
getUserAccessTokenwhen it throws on redirect.As discussed in
src/authConfig.ts, bothloginRedirectandacquireTokenRedirectpaths throwError('Redirecting to login').handleClickhas nocatchblock, so this propagates as an unhandled rejection. Thefinallyblock still runs, callingsetIsWorking(false)on a potentially unmounting component.Consider catching and ignoring the redirect sentinel, and suppressing the state-after-unmount case:
💡 Suggested fix
const handleClick = async () => { if (!pca) { return; } setIsWorking(true); try { const accessToken = await getUserAccessToken(pca, returnPath); await onAction(accessToken, showError); + } catch (e) { + if (e instanceof Error && e.message === 'Redirecting to login') return; + throw e; } finally { setIsWorking(false); } };src/components/store/StoreItem.tsx (1)
126-135:⚠️ Potential issue | 🟠 MajorBackground
getUserAccessTokencalls may unexpectedly redirect the user.
checkMembershipStatus(Line 128) and the preload effect (Line 204) callgetUserAccessToken(pca)without areturnPath. Under the old popup flow, anInteractionRequiredAuthErrorwould show a popup; now it callsacquireTokenRedirect, which navigates the user away from the page during a background operation they didn't initiate.While the
!userguard prevents calls when no account exists, token expiry or consent requirements can still triggerInteractionRequiredAuthErrorfor a known account.Consider wrapping these background calls to only attempt
acquireTokenSilentand silently fail on interaction-required errors, rather than redirecting:💡 Sketch: silent-only token helper
// In authConfig.ts or a local helper export const getUserAccessTokenSilentOnly = async ( pca: IPublicClientApplication ): Promise<string | null> => { const account = pca.getActiveAccount() || pca.getAllAccounts()[0]; if (!account) return null; try { const response = await pca.acquireTokenSilent({ ...loginRequest, account, }); return response.accessToken; } catch { return null; } };Then use it in
checkMembershipStatusand the preload effect instead ofgetUserAccessToken.Also applies to: 202-208
🤖 Fix all issues with AI agents
In `@e2e/home.spec.ts`:
- Around line 8-13: The test "Hero renders with heading" uses Playwright's
assertion without awaiting it: change the assertion on the locator variable
heading so the promise is awaited (i.e., add await before the
expect(...).toBeVisible() call) so the auto-retrying assertion actually runs;
update the same pattern if present in the related test resources.spec.ts.
In `@e2e/resources.spec.ts`:
- Around line 17-22: The async Playwright assertion call in the looped tests is
missing an await, so the promise returned by header.toContainText(heading) is
discarded and the test can pass incorrectly; update the test block inside the
for loop (the test callback that defines header via page.getByRole) to await the
assertion (await header.toContainText(heading)) so the auto-retrying assertion
runs and the test fails properly when the heading is absent.
In `@src/authConfig.ts`:
- Around line 40-42: The thrown Error('Redirecting to login') from the
pca.loginRedirect branches (the throw after await pca.loginRedirect(request) in
authConfig functions) causes an unhandled promise rejection; instead of
throwing, stop propagation by returning immediately after calling
pca.loginRedirect(request) (and do the same for the second identical throw), so
the caller (AuthActionButton.handleClick) won't receive an exception; locate the
throws referencing "Redirecting to login" and replace them with an early return
to avoid the noisy unhandled rejection.
In `@src/components/MembershipCheckButton.tsx`:
- Line 26: The returnPath currently uses window.location.href which produces
absolute URLs and can corrupt query strings; update the MembershipCheckButton
component to pass a relative path instead (consistent with other components) by
setting returnPath to a relative URL (e.g., "/membership?authButtonClick") or,
if you need to preserve the current path, build a safe relative path using
window.location.pathname + (window.location.search ? window.location.search +
"&authButtonClick" : "?authButtonClick") so the returnPath prop (used by the
OIDC state/redirect) is always a proper relative path and never produces a
malformed query string.
In `@src/pages/auth-redirect.astro`:
- Around line 10-20: Wrap the asynchronous redirect handling in a try/catch
around initMsalClient() and pca.handleRedirectPromise() to handle rejections
(use initMsalClient, handleRedirectPromise, pca.setActiveAccount), log or
surface the error and redirect to a safe fallback (e.g., '/login' or '/'); and
before calling window.location.replace(result.state || '/'), validate
result.state to prevent open redirects by ensuring it is either a relative path
(starts with '/') or same-origin (parse and compare origin), otherwise ignore it
and use the safe fallback. Ensure the catch block also falls back to the safe
redirect so users are not left on a spinner.
🧹 Nitpick comments (5)
src/components/calendar/CalendarControls.tsx (1)
38-39: Prefer a narrower cast overas any.
as anydisables all type checking. If the previousas stringworked, it's a safer cast. If the issue is thatlocalizer.addexpects a more specific type, consider casting to that specific type instead.- // eslint-disable-next-line `@typescript-eslint/no-explicit-any` - const candidate = localizer.add(displayDate, offset, unit as any); + const candidate = localizer.add(displayDate, offset, unit as string);e2e/identitySync.spec.ts (1)
16-26: Consider a less brittle URL assertion for the login redirect.The
waitForURLhardcodes the Azure AD tenant ID and client ID. If the auth configuration ever changes, this test silently becomes stale. A more resilient check would assert only the login domain:Suggested change
- await page.waitForURL( - 'https://login.microsoftonline.com/44467e6f-462c-4ea2-823f-7800de5434e3/oauth2/v2.0/authorize?client_id=d64e9c50-d144-4b4a-a315-ad2ed456c37**' - ); + await page.waitForURL('https://login.microsoftonline.com/**');e2e/resources.spec.ts (1)
5-7: Nit: Missing leading/ingotopath.Other test files consistently use absolute paths (e.g.,
/admin/sync,/). Considerpage.goto('/resources')for consistency.Proposed fix
- await page.goto('resources/'); + await page.goto('/resources');src/components/AuthActionButton.tsx (1)
50-60: MissinghandleClickinuseEffectdependency array — and potential stale closure.The effect references
handleClickbut only declares[pca]as a dependency. This will trigger anreact-hooks/exhaustive-depslint warning. More importantly, ifreturnPathoronActionprops change beforepcainitializes, the effect captures a stalehandleClick.In practice this is unlikely to cause issues (the effect runs once after MSAL init and the ref guard prevents re-runs), but it's worth being explicit.
♻️ Option: wrap handleClick in useCallback and include in deps
- const handleClick = async () => { + const handleClick = useCallback(async () => { if (!pca) { return; } setIsWorking(true); try { const accessToken = await getUserAccessToken(pca, returnPath); await onAction(accessToken, showError); } finally { setIsWorking(false); } - }; + }, [pca, returnPath, onAction, showError]); useEffect(() => { if (!pca || autoTriggered.current) return; const params = new URLSearchParams(window.location.search); if (!params.has('authButtonClick')) return; autoTriggered.current = true; params.delete('authButtonClick'); const qs = params.toString(); const newUrl = window.location.pathname + (qs ? `?${qs}` : ''); window.history.replaceState({}, '', newUrl); handleClick(); - }, [pca]); + }, [pca, handleClick]);e2e/membership.spec.ts (1)
24-27: Extract the duplicated login URL pattern into a shared constant to improve maintainability.The Microsoft login URL glob pattern is duplicated across test files—appearing twice in
membership.spec.ts(lines 25, 47) and once inidentitySync.spec.ts(line 24). If the tenant ID or client ID changes, multiple test files require updates.Create
e2e/helpers.ts:export const LOGIN_URL_PATTERN = 'https://login.microsoftonline.com/44467e6f-462c-4ea2-823f-7800de5434e3/oauth2/v2.0/authorize?client_id=d64e9c50-d144-4b4a-a315-ad2ed456c37**';Then import and use it in each spec file:
- await page.waitForURL( - 'https://login.microsoftonline.com/44467e6f-462c-4ea2-823f-7800de5434e3/oauth2/v2.0/authorize?client_id=d64e9c50-d144-4b4a-a315-ad2ed456c37**' - ); + await page.waitForURL(LOGIN_URL_PATTERN);
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @.github/workflows/deploy.yml:
- Around line 166-169: The workflow is using mismatched action versions and
Node.js target: update the uses entries for actions/checkout and
actions/setup-node to the same major version used elsewhere (change
actions/checkout@v5 and actions/setup-node@v5 to `@v4`) and set node-version to
the consistent value (change node-version: lts/* to node-version: 24.x) so all
jobs use the same action majors and Node.js runtime (look for the occurrences of
actions/checkout, actions/setup-node, and node-version in the deploy job).
- Around line 131-137: The CI is configured to upload blob reports from
blob-report/, but the Playwright config uses reporter: process.env.CI ? 'html' :
'list'; change the reporter to produce blob reports in CI by replacing that
expression with reporter: process.env.CI ? ['blob'] : 'list' (or equivalent
array form) in playwright.config.ts so Playwright writes to blob-report/,
ensuring the artifact upload step (which names blob-report-${{ matrix.shardIndex
}}) finds the expected files.
- Around line 161-189: The merge-reports job lacks a minimal permissions block
and can run when e2e was skipped; add a permissions block (e.g., minimal read
perms needed for artifacts/actions) to the merge-reports job and tighten its if
condition so it only runs when e2e actually ran successfully or for PRs; update
the job named merge-reports to include a permissions entry (grant only the least
privileges required for downloading/uploading artifacts) and change the
job-level if from "${{ !cancelled() }}" to a compound guard such as "${{
!cancelled() && needs.e2e.result == 'success' }}" or scope it to pull_request
events (e.g., check github.event_name == 'pull_request') so the "Download blob
reports" step won't fail when there are no artifacts.
1 participant
Summary by CodeRabbit
New Features
Refactor
Tests
Chores