fix(identity): OAuth error handling, ConnectionHub UX, and portal improvements#290
Merged
Conversation
…ntials gracefully Prevents 500 errors when: Discord/GitHub/Twitch return error payloads instead of access tokens, user denies OAuth prompt (null code), or provider credentials are not configured in production.
…rapper Add dark: prefixed variants to all hardcoded dark-mode colors so the component renders correctly in both light and dark themes.
AttachProviderToUser now resets disconnected_at to null on updateOrCreate, ensuring reconnected providers show as active in both Login and Link flows.
…vatar key - Add Twitch as OAuth login provider on the login page - Add outlined "Área do Usuário" button to portal navbar linking to /app - Fix HeroSection crash: metadata key is 'username' not 'name'
…ero avatars Legacy records use 'name', new records use 'username'. Try both keys so the query returns results from all 2590 GitHub identities.
gvieira18
previously approved these changes
May 26, 2026
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR standardizes OAuth token-exchange failure handling with OAuthFlowException::tokenExchangeFailed, validates token responses in Discord/GitHub/Twitch clients, hardens OAuthController redirect/callback flows with logging and safe redirects, validates GitHub OAuth config at registration, adds a Twitch login button, updates responsive navbar and connection-hub UI styles, refines avatar/profile display logic, initializes ExternalIdentity.disconnected_at to null on attach, and converts many tenant_id columns in migrations to UUID semantics. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app-modules/identity/src/Auth/Http/Controllers/OAuthController.php`:
- Around line 66-72: In the OAuthController catch block for OAuthFlowException
(where $action->execute(...) is called and Log::warning is used), add a
user-facing error notification before redirecting: attach a flash/error message
to the redirect (e.g. redirect()->to($state->returnUrl ?? '/')->with('error',
'<friendly message>')) so users see an authentication failure notice while
keeping the existing logging and redirect behavior; keep the Log::warning call
with $oAuthFlowException->getMessage() unchanged.
- Around line 57-64: In OAuthController where $code and $oauthDenied are
determined, add a user-facing flash message when $oauthDenied is true (e.g., use
session()->flash or redirect()->with) so the redirect()->to($fallbackUrl)
communicates "Authentication was cancelled" (or similar); modify the block that
checks $oauthDenied (using $code, $oauthDenied, and $state->returnUrl) to set
the flash message before returning the redirect.
In `@app-modules/integration-github/src/IntegrationGithubServiceProvider.php`:
- Line 20: The throw_if check currently only rejects nulls for $clientId and
$clientSecret; update the condition in the throw_if call (the line referencing
RuntimeException::class and variables $clientId/$clientSecret) to also treat
empty strings (recommend checking empty(trim($clientId)) and
empty(trim($clientSecret)) or $clientId === ''/$clientSecret === '') as invalid
so the RuntimeException is thrown for missing credentials rather than letting
empty env values slip through.
In `@app-modules/portal/resources/views/components/navbar.blade.php`:
- Line 37: The Tailwind important modifier is currently placed after the utility
in the class attribute (see the class string containing "w-auto! md:hidden");
move the `!` so it precedes the utility (i.e., change the class token from the
incorrect "w-auto!" form to the correct "!w-auto" form) leaving other classes
like "md:hidden" unchanged to restore proper Tailwind behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 546e6b97-e208-41c1-b3b0-6634cd3cbb62
📒 Files selected for processing (12)
app-modules/identity/src/Auth/Actions/AttachProviderToUser.phpapp-modules/identity/src/Auth/Exceptions/OAuthFlowException.phpapp-modules/identity/src/Auth/Http/Controllers/OAuthController.phpapp-modules/integration-discord/src/OAuth/DiscordOAuthClient.phpapp-modules/integration-github/src/IntegrationGithubServiceProvider.phpapp-modules/integration-github/src/OAuth/GitHubOAuthClient.phpapp-modules/integration-twitch/src/OAuth/TwitchOAuthClient.phpapp-modules/panel-app/resources/views/auth/login.blade.phpapp-modules/panel-app/resources/views/pages/profile.blade.phpapp-modules/portal/resources/views/components/navbar.blade.phpapp-modules/portal/src/Livewire/HeroSection.phpresources/views/livewire/connection-hub.blade.php
… current schema
All foreignId('tenant_id') changed to foreignUuid('tenant_id') and
foreignIdFor references to UUID tables updated. Also restores the
missing set_up_tenant_module migration that adds tenant_id to legacy
tables (providers, messages, characters, etc).
gvieira18
approved these changes
May 26, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
namevsusernamefor legacy/new GitHub records)disconnected_atwhen reconnecting an existing providerChanges
Identity module
OAuthController: catchOAuthFlowExceptionon token exchange, redirect with error notificationAttachProviderToUser: cleardisconnected_aton reconnectOAuthFlowExceptionon missing credentialsPanel App
ConnectionHub: compact card layout, dark/light theme support, pill-style disconnect button, timestamp next to titleLoginPage: add Twitch OAuth buttonbg-white dark:bg-gray-900)Portal
/appHeroSection: support bothusername(new) andname(legacy) metadata keys for GitHub avatarsTest plan
/appDescription
This PR improves OAuth robustness and UX: it adds explicit token-exchange error handling and credential validation for OAuth providers, clears disconnected provider state on reconnection, adds Twitch as a login option, and improves ConnectionHub/profile/portal UI and avatar metadata handling to prevent avatar-loading errors and support light/dark themes.
References
#288: OAuth flow refactor with account merge strategy — feat(identity): OAuth flow refactor with account merge strategy #288#272: Twitch integration and EventSub foundation — feat(integration-twitch): Twitch integration with admin panel, tenant-scoped EventSub, and subscription management #272#289: Data migration indexes — fix: data migration indexes #289Dependencies & Requirements
Contributor Summary
Changes Summary
disconnected_at(set to null) on provider attach/reconnect to reactivate provider