Skip to content

Do now show login error before flow starts#392

Merged
celdrake merged 1 commit into
flightctl:mainfrom
celdrake:do-not-show-error-before-login-flow
Nov 27, 2025
Merged

Do now show login error before flow starts#392
celdrake merged 1 commit into
flightctl:mainfrom
celdrake:do-not-show-error-before-login-flow

Conversation

@celdrake

@celdrake celdrake commented Nov 27, 2025

Copy link
Copy Markdown
Collaborator

When the user first went to the UI page, we'd briefly show an error as "getUserInfo" returned statusCode 401.
Now we will start the login flow and the errors will be shown if the login fails.

Summary by CodeRabbit

  • Bug Fixes
    • Improved authentication error handling for unauthorized access scenarios. The app now more efficiently manages login redirects when encountering authentication failures outside of authentication pages, ensuring a smoother user experience.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai

coderabbitai Bot commented Nov 27, 2025

Copy link
Copy Markdown

Walkthrough

Modified HTTP 401 error handling in the authentication context to immediately initiate login flow when not on login or callback pages, replacing previous logic that extracted error messages before conditionally redirecting.

Changes

Cohort / File(s) Summary
Authentication Error Handling
apps/standalone/src/app/context/AuthContext.ts
Refactored 401 unauthorized response handling to check if current page is login/callback page; if not, immediately sets loading to false and calls redirectToLogin with early return, removing previous error extraction and conditional redirect logic.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

  • Ensure page detection logic correctly identifies login/callback pages across all routing scenarios
  • Verify early redirect doesn't suppress important error information needed for debugging
  • Confirm loading state transitions are correct and don't cause UI inconsistencies
  • Check that existing authentication flows remain unbroken with the reordered logic

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title contains a grammatical error ('Do now show' instead of 'Do not show') but clearly conveys the main change: preventing login errors from displaying prematurely.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
apps/standalone/src/app/context/AuthContext.ts (1)

104-111: 401 handling now matches the desired login-flow behavior

The new isOnAuthPage check and early return on 401 correctly avoid flashing an authentication error on initial navigation: non-auth routes redirect straight to /login without setting error, while /callback still shows a meaningful error if /info returns 401 after a failed login.

One small optional cleanup: since the hook exits early on Line 36 when window.location.pathname === '/login', the /login case will never reach this 401 block, so including '/login' in isOnAuthPage is redundant. You could simplify it to just window.location.pathname === '/callback' or extract a shared isOnAuthPage() helper used in both places, but this is purely cosmetic.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f42047f and 89270b8.

📒 Files selected for processing (1)
  • apps/standalone/src/app/context/AuthContext.ts (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 376
File: libs/ui-components/src/components/Repository/CreateRepository/CreateRepositoryForm.tsx:274-275
Timestamp: 2025-11-17T15:53:01.194Z
Learning: In PR #376 (flightctl/flightctl-ui), the permission system was updated to fetch all user permissions once at login via the Flight Control API. The `checkPermissions` function from `usePermissionsContext` is a pure calculation on cached permission arrays, not an async fetch, so loading state handling is generally not required at individual component call sites.
Learnt from: rawagner
Repo: flightctl/flightctl-ui PR: 209
File: apps/standalone/src/app/utils/apiCalls.ts:14-16
Timestamp: 2025-02-18T12:04:42.579Z
Learning: In the flightctl-ui codebase, error handling for API calls (like logout) is implemented by the caller rather than within the API utility functions themselves.
Learnt from: rawagner
Repo: flightctl/flightctl-ui PR: 209
File: apps/standalone/src/app/utils/apiCalls.ts:14-16
Timestamp: 2025-02-18T12:04:42.579Z
Learning: In the flightctl-ui codebase, error handling for API calls follows a React pattern where the caller component (e.g., AppToolbar.tsx) implements try-catch blocks and manages error states, rather than handling errors within the API utility functions themselves.
🧬 Code graph analysis (1)
apps/standalone/src/app/context/AuthContext.ts (1)
apps/standalone/src/app/utils/apiCalls.ts (1)
  • redirectToLogin (58-60)
⏰ 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). (4)
  • GitHub Check: integration-tests
  • GitHub Check: Build ocp plugin
  • GitHub Check: Build
  • GitHub Check: Lint

@celdrake celdrake merged commit 55d52de into flightctl:main Nov 27, 2025
6 checks passed
@celdrake celdrake deleted the do-not-show-error-before-login-flow branch November 27, 2025 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants