feat(chat): Implement authentication flow with login UI and session management #8625
Conversation
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
🤖 AI PR Validation ReportPR Review ResultsThank you for your submission! Here's detailed feedback on your PR title and body compliance:✅ PR Title
✅ Commit Type
❌ Risk Level
✅ What & Why
✅ Impact of Change
✅ Test Plan
|
| Section | Status | Recommendation |
|---|---|---|
| Title | ✅ | |
| Commit Type | ✅ | |
| Risk Level | ❌ | Update to High in body & label |
| What & Why | ✅ | |
| Impact of Change | ✅ | |
| Test Plan | ✅ | |
| Contributors | Tag relevant collaborators if any | |
| Screenshots/Videos | Add screenshot/gif of new UI if possible |
Please update the PR body to set Risk Level to HIGH if you agree with the above assessment, and set the GitHub label to risk:high. Also, consider including a screenshot/video for the new login experience. Thank you for your thorough documentation and coverage on this high-impact change!
Last updated: Wed, 03 Dec 2025 01:52:36 GMT
ccastrotrejo
changed the title
Copilot finished reviewing on behalf of
ccastrotrejo
December 2, 2025 23:29
Contributor
There was a problem hiding this comment.
Pull request overview
This PR implements authentication functionality for the iframe chat application, introducing login/logout handling through Azure App Service EasyAuth. The changes include refactoring the authentication flow, restructuring the SessionList component into separate files for better maintainability, and adding a new LoginPrompt component for user authentication.
Key Changes
- Authentication Implementation: Added
openLoginPopup()function andcheckAuthStatus()to handle Azure AD login flows with popup-based authentication - Component Restructuring: Split monolithic SessionList component into modular files (SessionList.tsx, SessionItem.tsx, SessionListStyles.ts) following Fluent UI v9 patterns
- Auth Flow Integration: Updated
createUnauthorizedHandlerto trigger login UI instead of automatic logout, integrated with IframeWrapper and MultiSessionChat components
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| turbo.json | Added dist/** to build:lib outputs for library distribution |
| apps/iframe-app/src/lib/utils/config-parser.ts | Added getAgentBaseUrl() helper to extract base URL from agent card URLs |
| apps/iframe-app/src/lib/utils/tests/config-parser.test.ts | Added comprehensive tests for getAgentBaseUrl() covering edge cases |
| apps/iframe-app/src/lib/index.tsx | Removed unused CSS import for logicAppsChat styles |
| apps/iframe-app/src/lib/iframe.tsx | Removed unused CSS import for logicAppsChat styles |
| apps/iframe-app/src/lib/authHandler.ts | Refactored authentication: replaced logout flow with login popup, added openLoginPopup() and checkAuthStatus() functions |
| apps/iframe-app/src/lib/tests/authHandler.test.ts | Added tests for new auth functions, but tests contain critical bugs (incorrect callback names, mismatched expectations) |
| apps/iframe-app/src/components/SessionList/SessionListStyles.ts | New file: Fluent UI v9 styles for SessionList (migrated from CSS modules) |
| apps/iframe-app/src/components/SessionList/SessionList.tsx | Extracted SessionList component from monolithic file, using makeStyles pattern |
| apps/iframe-app/src/components/SessionList/SessionItem.tsx | Extracted SessionItem component for better modularity and performance |
| apps/iframe-app/src/components/SessionList.tsx | Deleted: Split into separate modular files |
| apps/iframe-app/src/components/SessionList.module.css | Deleted: Migrated to Fluent UI v9 makeStyles |
| apps/iframe-app/src/components/MultiSessionChat/MultiSessionChatWithAuth.tsx | Updated import paths after component restructuring |
| apps/iframe-app/src/components/MultiSessionChat/MultiSessionChatStyles.ts | New file: Extracted styles using makeStyles pattern |
| apps/iframe-app/src/components/MultiSessionChat/MultiSessionChat.tsx | Refactored to use extracted styles, added 401 handling for agent card fetch, removed FluentProvider wrapper |
| apps/iframe-app/src/components/MultiSessionChat.module.css | Deleted: Migrated to makeStyles |
| apps/iframe-app/src/components/LoginPrompt/index.ts | New file: Exports for LoginPrompt component |
| apps/iframe-app/src/components/LoginPrompt/LoginPromptStyles.ts | New file: Styles for login prompt using makeStyles |
| apps/iframe-app/src/components/LoginPrompt/LoginPrompt.tsx | New component: Login UI shown when authentication is required |
| apps/iframe-app/src/components/IframeWrapper.tsx | Integrated authentication flow with LoginPrompt, added FluentProvider at top level, uses new auth handlers |
Comments suppressed due to low confidence (3)
apps/iframe-app/src/components/MultiSessionChat/MultiSessionChat.tsx:157
- Checking
response.statusText === 'Unauthorized'is unreliable. HTTP status text varies between servers and may be empty or different strings. Useresponse.status === 401instead to reliably detect unauthorized responses.
apps/iframe-app/src/components/MultiSessionChat/MultiSessionChat.tsx:187 - The dependency array includes both
configand individual propertiesconfig.apiKey,config.apiUrl, andconfig.oboUserToken. Including the parent object along with its properties is redundant and can cause unnecessary re-renders. Removeconfigfrom the dependency array and keep only the specific properties that the effect uses:[config.apiKey, config.apiUrl, config.oboUserToken, config.onUnauthorized].
apps/iframe-app/src/components/MultiSessionChat/MultiSessionChat.tsx:161 - When the response status is 401 (Unauthorized), the code calls
onUnauthorized()but doesn't setsetIsLoadingAgent(false)or clear the loading state. This will leave the UI in a perpetual loading state. After callingonUnauthorized(), either return early or set an appropriate error state so the UI can respond appropriately.
Comment on lines
+186
to
+205
| it('should call onLoginSuccess when login popup succeeds', async () => { | ||
| vi.useFakeTimers(); | ||
|
|
||
| mockFetch | ||
| .mockResolvedValueOnce({ ok: false }) // refresh fails | ||
| .mockResolvedValueOnce({ | ||
| // checkAuthStatus succeeds | ||
| ok: true, | ||
| json: () => Promise.resolve([{ provider_name: 'aad' }]), | ||
| }); | ||
|
|
||
| const mockPopup = { closed: false, close: vi.fn(), location: { href: '' } }; | ||
| mockWindowOpen.mockReturnValueOnce(mockPopup); | ||
|
|
||
| const onLoginSuccess = vi.fn(); | ||
|
|
||
| // Simulate popup being closed | ||
| const handler = createUnauthorizedHandler({ | ||
| baseUrl: 'https://example.com', | ||
| onLoginSuccess, | ||
| }); |
There was a problem hiding this comment.
The test expects onLoginSuccess callback, but AuthHandlerConfig interface doesn't define this callback. The implementation calls onLoginRequired() when refresh fails, not when login succeeds. This test doesn't match the actual implementation behavior and will fail.
Comment on lines
+220
to
265
| it('should fall back to logout when login fails', async () => { | ||
| vi.useFakeTimers(); | ||
|
|
||
| mockFetch.mockResolvedValueOnce({ ok: false }); | ||
| const mockPopup = { | ||
| closed: false, | ||
| close: vi.fn(), | ||
| location: { href: 'https://example.com/.auth/logout' }, | ||
| }; | ||
| mockWindowOpen.mockReturnValueOnce(mockPopup); | ||
| mockFetch | ||
| .mockResolvedValueOnce({ ok: false }) // refresh fails | ||
| .mockResolvedValueOnce({ | ||
| // checkAuthStatus fails (not authenticated) | ||
| ok: true, | ||
| json: () => Promise.resolve([]), | ||
| }); | ||
|
|
||
| const mockLoginPopup = { closed: false, close: vi.fn(), location: { href: '' } }; | ||
| const mockLogoutPopup = { closed: false, close: vi.fn(), location: { href: '' } }; | ||
| mockWindowOpen.mockReturnValueOnce(mockLoginPopup).mockReturnValueOnce(mockLogoutPopup); | ||
|
|
||
| const onLoginFailed = vi.fn(); | ||
| const onLogoutComplete = vi.fn(); | ||
|
|
||
| const handler = createUnauthorizedHandler({ | ||
| baseUrl: 'https://example.com', | ||
| onLoginFailed, | ||
| onLogoutComplete, | ||
| }); | ||
|
|
||
| await handler(); | ||
|
|
||
| // Change popup URL to completion URL | ||
| mockPopup.location.href = 'https://example.com/.auth/logout/complete'; | ||
| // Simulate login popup being closed without success | ||
| mockLoginPopup.closed = true; | ||
|
|
||
| // Advance timers to trigger interval check | ||
| vi.advanceTimersByTime(600); | ||
| // Advance timers to trigger interval check and auth status check | ||
| await vi.advanceTimersByTimeAsync(600); | ||
|
|
||
| expect(onLoginFailed).toHaveBeenCalled(); | ||
|
|
||
| // Now logout popup should be opened | ||
| expect(mockWindowOpen).toHaveBeenCalledTimes(2); | ||
| expect(mockWindowOpen).toHaveBeenLastCalledWith('https://example.com/.auth/logout', 'auth-logout', 'width=600,height=700,popup=true'); | ||
|
|
||
| // Simulate logout popup being closed | ||
| mockLogoutPopup.closed = true; | ||
|
|
||
| await vi.advanceTimersByTimeAsync(600); | ||
|
|
||
| expect(mockPopup.close).toHaveBeenCalled(); | ||
| expect(onLogoutComplete).toHaveBeenCalled(); | ||
|
|
||
| vi.useRealTimers(); |
There was a problem hiding this comment.
This test expects the unauthorized handler to open a logout popup when login fails, but the implementation (authHandler.ts lines 263-293) only calls onLoginRequired() when refresh fails - it doesn't handle login popup success/failure or open logout popups. The test logic doesn't match the actual implementation.
Comment on lines
130
to
+276
| mockFetch.mockResolvedValueOnce({ ok: false }); | ||
| mockWindowOpen.mockReturnValueOnce(null); | ||
|
|
||
| const onLoginFailed = vi.fn(); | ||
|
|
||
| const handler = createUnauthorizedHandler({ | ||
| baseUrl: 'https://example.com', | ||
| onLoginFailed, |
There was a problem hiding this comment.
The test expects onLoginFailed callback, but AuthHandlerConfig interface doesn't define this callback. The test should use onLoginRequired instead, which is the actual callback that exists in the interface.
| icon={<AddFilled fontSize={16} />} | ||
| onClick={onNewSession} | ||
| size="medium" | ||
| title="New chat" |
There was a problem hiding this comment.
[nitpick] The button title uses lowercase "New chat" while the button text uses "New chat" (title case). For consistency with UI standards and accessibility (screen readers will read the title), both should use the same case. Consider changing the title to match: title="New Chat".
Suggested change
| title="New chat" | |
| title="New Chat" |
| expect(result).toBe(true); | ||
| expect(mockFetch).toHaveBeenCalledWith('https://example.com/.auth/me', { | ||
| method: 'GET', | ||
| credentials: 'same-origin', |
There was a problem hiding this comment.
The test expects credentials: 'same-origin', but the implementation in authHandler.ts line 245 uses credentials: 'include'. This mismatch will cause the test to fail. The implementation should use 'include' for cross-origin cookie support, so the test expectation should be updated to match.
Suggested change
| credentials: 'same-origin', | |
| credentials: 'include', |
Comment on lines
+162
to
174
| it('should open login popup when refresh fails', async () => { | ||
| mockFetch.mockResolvedValueOnce({ ok: false }); | ||
| const mockPopup = { closed: false, close: vi.fn(), location: { href: '' } }; | ||
| mockWindowOpen.mockReturnValueOnce(mockPopup); | ||
|
|
||
| const onRefreshFailed = vi.fn(); | ||
| const onLogoutComplete = vi.fn(); | ||
| const onLoginSuccess = vi.fn(); | ||
|
|
||
| const handler = createUnauthorizedHandler({ | ||
| baseUrl: 'https://example.com', | ||
| onRefreshFailed, | ||
| onLogoutComplete, | ||
| onLoginSuccess, | ||
| }); |
There was a problem hiding this comment.
The test expects onLoginSuccess and onLoginFailed callbacks, but the AuthHandlerConfig interface (line 11 in authHandler.ts) only defines onLoginRequired, not onLoginSuccess or onLoginFailed. This test will fail because the handler implementation doesn't support these callbacks. The test should be updated to use the actual onLoginRequired callback.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Commit Type
Risk Level
What & Why
This PR implements a comprehensive authentication flow for the iframe chat application. It adds:
LoginPromptcomponent with Fluent UI styling that displays when users need to authenticateThe changes improve the user experience by providing a clear, branded login interface instead of abrupt redirects when authentication is required.
Impact of Change
LoginPromptcomponent available for reuseauthHandlerAPI withonLoginRequiredcallback replacingonLogoutCompleteapps/iframe-app/src/components/with subdirectories forMultiSessionChatandSessionListopenLoginPopupandcheckAuthStatusfunctionsTest Plan
Tests Updated:
authHandler.test.tswith new test cases foropenLoginPopupandcheckAuthStatusconfig-parser.test.tswith comprehensive tests forgetAgentBaseUrlutility functionManual Testing:
Contributors
@ccastrotrejo
Screenshots/Videos
New
LoginPromptcomponent that displays when authentication is required: