Fix disconnect social auth bugs #1600
Conversation
WalkthroughConnections.tsx: added hydration-aware client flow, typed Providers list, per-provider connection state, a disconnectingProviderRef, revalidation and toast handling on disconnect success/failure, improved ApiError message extraction, and builds provider auth links with nextUrl. Settings.tsx: renamed default export to UserSettings, removed server-side NotLoggedIn guard and loader, uses outlet context and pathname to compute active tab with a tabClass helper, and imports Styles. Connection.tsx: ConnectionProps now requires Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1600 +/- ##
=========================================
- Coverage 9.88% 9.87% -0.01%
=========================================
Files 310 310
Lines 22497 22514 +17
Branches 406 406
=========================================
Hits 2223 2223
- Misses 20274 20291 +17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/cyberstorm-remix/app/settings/user/Connections/Connections.tsx(2 hunks)apps/cyberstorm-remix/app/settings/user/Settings.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/cyberstorm-remix/app/settings/user/Settings.tsx (3)
apps/cyberstorm-remix/app/root.tsx (1)
OutletContextShape(84-89)apps/cyberstorm-remix/cyberstorm/security/publicEnvVariables.ts (1)
getPublicEnvVariables(19-48)packages/dapper-ts/src/index.ts (1)
DapperTs(38-96)
apps/cyberstorm-remix/app/settings/user/Connections/Connections.tsx (1)
packages/thunderstore-api/src/errors.ts (1)
ApiError(14-64)
⏰ 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). (2)
- GitHub Check: Generate visual diffs
- GitHub Check: Build
Roffenlund
force-pushed
the
fix-disconnect-social-auth-bugs
branch
from
57b7d24 to
1663a1d
Compare
Roffenlund
changed the title
anttimaki
left a comment
anttimaki
left a comment
There was a problem hiding this comment.
I think there's a bug on "prevent removing last connection".
Other comments are not blockers, they're mostly focused on finding common practices that should be made code base wide.
apps/cyberstorm-remix/app/settings/user/Connections/Connections.tsx
Outdated
Show resolved
Hide resolved
Roffenlund
force-pushed
the
fix-disconnect-social-auth-bugs
branch
from
1663a1d to
003bebd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/cyberstorm-remix/app/commonComponents/Connection/Connection.tsx(2 hunks)apps/cyberstorm-remix/app/settings/user/Connections/Connections.tsx(3 hunks)apps/cyberstorm-remix/app/settings/user/Settings.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/cyberstorm-remix/app/settings/user/Connections/Connections.tsx (4)
apps/cyberstorm-remix/app/root.tsx (1)
OutletContextShape(84-89)packages/thunderstore-api/src/errors.ts (1)
ApiError(14-64)apps/cyberstorm-remix/app/commonComponents/Connection/Connection.tsx (1)
Connection(17-48)apps/cyberstorm-remix/cyberstorm/utils/ThunderstoreAuth.tsx (1)
buildAuthLoginUrl(8-14)
apps/cyberstorm-remix/app/settings/user/Settings.tsx (4)
apps/cyberstorm-remix/app/root.tsx (1)
OutletContextShape(84-89)apps/cyberstorm-remix/cyberstorm/security/publicEnvVariables.ts (1)
getSessionTools(50-67)packages/cyberstorm/src/index.ts (2)
Tabs(109-109)SkeletonBox(92-92)packages/cyberstorm/src/newComponents/Tabs/Tabs.tsx (1)
Tabs(18-40)
⏰ 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). (2)
- GitHub Check: Test
- GitHub Check: Generate visual diffs
| const getConnection = (provider: ProvidersType) => | ||
| outletContext.currentUser?.connections?.find( | ||
| (c) => c.provider?.toLowerCase() === provider.identifier | ||
| ); |
There was a problem hiding this comment.
Fix provider typing in getConnection.
Parameter is typed as ProvidersType (the whole array), so identifier is missing and the file won’t compile. Please type it as a single provider entry instead.
- const getConnection = (provider: ProvidersType) =>
+ const getConnection = (provider: ProvidersType[number]) =>🤖 Prompt for AI Agents
In apps/cyberstorm-remix/app/settings/user/Connections/Connections.tsx around
lines 54 to 57, the getConnection parameter is incorrectly typed as the whole
ProvidersType array; change the parameter to represent a single provider entry
(e.g. ProvidersType[number] or the specific Provider type used in the codebase)
so provider.identifier exists and the file compiles, then keep the comparison
logic (provider.identifier) as-is.
Fix how the error message is fetched and displayed when disconnecting a social auth account. Refs. TS-2758
Add disable flag to render a connection switch disabled. Refs. TS-2758
Simplify Settings parent component to be a simple component holding no logic. Create helper class for getting current tab class. Refs. TS-2758
Update logic for handling removing social auth accounts. - Use Suspend/Await pattern in component - Always fetch new user in clientLoader & update session - Revalidate when successful disconnect - Use HydrateFallback in order to properly render loading state - Keep track of disconnected social auths - Prevent user from disconnecting last social auth account - Show proper success message to user Refs. TS-2758
Roffenlund
force-pushed
the
fix-disconnect-social-auth-bugs
branch
from
003bebd to
15ddefd
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/cyberstorm-remix/app/settings/user/Connections/Connections.tsx (1)
72-75: FixgetConnectionparameter typing (array vs single element)
getConnectionis currently typed as takingprovider: ProvidersType, butProvidersTypeis the array type. Inside the function you accessprovider.identifier, which doesn’t exist on the array type and will fail TypeScript checks.Change it to a single provider entry, for example:
const getConnection = (provider: ProvidersType[number]) => outletContext.currentUser?.connections?.find( (c) => c.provider?.toLowerCase() === provider.identifier );That matches how it’s used in
PROVIDERS.map((provider) => …)and will compile correctly.
🧹 Nitpick comments (5)
apps/cyberstorm-remix/app/settings/user/Settings.tsx (1)
7-14: Consider making tab detection andaria-currenta bit more robustThe current tab is inferred via
pathname.endsWith("/account/"), andaria-currentis passed a boolean. This works given the current routing, but it’s a bit brittle if:
- Routes ever lose the trailing slash or get deeper under
/account/....- You want more explicit ARIA semantics.
You could, for example, match on
pathname.includes("/account")or use route info instead of raw string suffixes, and setaria-current={currentTab === "account" ? "page" : undefined}(same for"settings"). That keeps the visual logic intact while making the behavior more tolerant to route changes and a bit clearer for assistive tech.Also applies to: 24-30, 33-39, 44-45
apps/cyberstorm-remix/app/settings/user/Connections/Connections.tsx (4)
27-33: Avoid deep relative import for API endpoint if a package export exists
userLinkedAccountDisconnectis imported via a long relative path intopackages/thunderstore-api/src. If this endpoint is (or can be) exported from@thunderstore/thunderstore-apior a similar package entrypoint, prefer that to keep consumer code decoupled from the library’s internal layout and to reduce risk from refactors in the packages folder.If it’s not exported yet, consider adding a proper export there and switching this import over when convenient.
49-57: UseHydrateFallback(or similar) as the Suspense/Await pending UIYou’ve added a
HydrateFallbackcomponent but<Suspense>and<Await>currently don’t use it as a pending UI (nofallbackon<Suspense>, nopendingElementon<Await>). That means subsequent client-side loads/revalidations will render nothing whilecurrentUseris pending.Consider wiring the fallback through, e.g.:
<Suspense key="user-settings-connections" fallback={<HydrateFallback />}> <Await resolve={currentUser} errorElement={<div>Error loading user</div>} > … </Await> </Suspense>(or by using
pendingElementon<Await>), so users always see a clear loading state.Also applies to: 128-131
78-95: Consider showing the human‑friendly provider name in the success toast
disconnectingProviderRef.currentstores the provider identifier (e.g."github"), so the toast message will read likeUser alice was disconnected from github. Since you already havenamefields inPROVIDERS, you could upgrade this to a nicer label.For example, store the display name instead:
disconnectingProviderRef.current = PROVIDERS.find( (p) => p.identifier === provider )?.name ?? provider;and keep the rest of the toast logic as-is. That better matches the PR goal of a “proper success message including the provider name.”
60-61: TypeuseOutletContextgenerically instead of castingMinor style point: instead of
const outletContext = useOutletContext() as OutletContextShape;you can let TypeScript infer it via the generic:
const outletContext = useOutletContext<OutletContextShape>();Functionally equivalent, but avoids the
ascast and keeps the type tied directly to the hook.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/cyberstorm-remix/app/commonComponents/Connection/Connection.tsx(2 hunks)apps/cyberstorm-remix/app/settings/user/Connections/Connections.tsx(3 hunks)apps/cyberstorm-remix/app/settings/user/Settings.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/cyberstorm-remix/app/commonComponents/Connection/Connection.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
apps/cyberstorm-remix/app/settings/user/Settings.tsx (3)
apps/cyberstorm-remix/app/root.tsx (1)
OutletContextShape(84-89)apps/cyberstorm-remix/app/commonComponents/PageHeader/PageHeader.tsx (1)
PageHeader(17-81)packages/cyberstorm/src/newComponents/Tabs/Tabs.tsx (1)
Tabs(18-40)
apps/cyberstorm-remix/app/settings/user/Connections/Connections.tsx (5)
apps/cyberstorm-remix/app/root.tsx (1)
OutletContextShape(84-89)apps/cyberstorm-remix/cyberstorm/security/publicEnvVariables.ts (1)
getSessionTools(50-67)packages/thunderstore-api/src/errors.ts (1)
ApiError(14-64)apps/cyberstorm-remix/app/commonComponents/Connection/Connection.tsx (1)
Connection(17-48)apps/cyberstorm-remix/cyberstorm/utils/ThunderstoreAuth.tsx (1)
buildAuthLoginUrl(8-14)
⏰ 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). (2)
- GitHub Check: Build
- GitHub Check: Generate visual diffs
🔇 Additional comments (3)
apps/cyberstorm-remix/app/settings/user/Connections/Connections.tsx (3)
53-56: Overall loader + revalidation wiring looks solidThe
clientLoader+useLoaderData+Awaitflow, combined withrevalidator.revalidate()on successful disconnect, is a clean pattern for keepingcurrentUser(and thus connections) fresh after mutations. UsingbuildAuthLoginUrlwith public env variables to construct connect URLs is also consistent with the surrounding utilities.No functional issues spotted here.
Also applies to: 60-65, 123-131, 157-183
66-70:onlyOneConnectedcorrectly guards against disconnecting the last providerThe
onlyOneConnected()helper combined withdisabled={onlyOneConnected() && !!getConnection(provider)}does what you intend: it only disables the switch for the last connected provider, while still allowing users to connect additional providers.This fixes the earlier concern about blocking users from adding a second social auth when they already have one.
Also applies to: 157-161
97-108: Error handling and user‑facing messages are in good shapeDeriving the error message from
ApiError.getFieldErrors()with sensible fallbacks and surfacing it via a danger toast is a solid improvement over generic failures. The prioritization ofnon_field_errors,detail, thenroot, thenerror.messageshould cover common API error shapes without exposing raw JSON.Looks good as-is.
Also applies to: 110-114
- Remove loaders as the data is available in the outletContext.currentUser, and it's always up-to-date, becuase settings pages path cause a revalidation in root loaders - Remove usage of Suspense/Await as it's not needed - Add usage of isHydrated to display "loading" element while client hydration is on going - Add correct nextUrl into the buildAuthLoginUrl function props
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/cyberstorm-remix/app/settings/user/Connections/Connections.tsx (1)
112-114: Consider using a design system loading component.The inline loading message doesn't match the app's design patterns. If a loading skeleton or spinner component exists, use it here for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/cyberstorm-remix/app/settings/user/Connections/Connections.tsx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/cyberstorm-remix/app/settings/user/Connections/Connections.tsx (4)
packages/cyberstorm/src/newComponents/Toast/Provider.tsx (1)
useToast(79-87)packages/thunderstore-api/src/errors.ts (1)
ApiError(14-64)apps/cyberstorm-remix/app/commonComponents/Connection/Connection.tsx (1)
Connection(17-48)apps/cyberstorm-remix/cyberstorm/utils/ThunderstoreAuth.tsx (1)
buildAuthLoginUrl(8-14)
⏰ 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). (2)
- GitHub Check: Build
- GitHub Check: Generate visual diffs
🔇 Additional comments (6)
apps/cyberstorm-remix/app/settings/user/Connections/Connections.tsx (6)
1-20: LGTM!Import additions support the new hydration-aware flow, revalidation, and improved error handling.
22-40: LGTM!Provider type definitions are explicit and correct. The array typing ensures each provider has the required structure.
42-48: LGTM!Component hooks are properly initialized for the hydration-aware disconnect flow.
49-58: LGTM!Helper functions correctly determine connection state and when to disable disconnect actions.
139-163: LGTM!The provider rendering correctly:
- Disables disconnect when it's the last connection
- Returns users to settings after re-authentication
- Tracks the disconnecting provider for success messaging
80-98: No changes needed.The error handling is correctly implemented and defensive.
getFieldErrors()generically passes through any fields from the backend response, converting them to string arrays. The code then safely checks for common field names (non_field_errors,detail,root) with proper fallbacks—this approach works regardless of the exact error structure the backend returns.
| const onSubmitSuccess = (_r: unknown) => { | ||
| if (!outletContext.currentUser || !outletContext.currentUser.username) | ||
| throw new Error("User not logged in"); | ||
| const username = outletContext.currentUser?.username; | ||
|
|
||
| revalidator.revalidate(); | ||
|
|
||
| toast.addToast({ | ||
| csVariant: "success", | ||
| children: ( | ||
| <> | ||
| User {outletContext.currentUser.username} was disconnected from TODO | ||
| User {username} was disconnected from{" "} | ||
| {disconnectingProviderRef.current} | ||
| </> | ||
| ), | ||
| duration: 30000, | ||
| }); | ||
|
|
||
| disconnectingProviderRef.current = null; | ||
| }; |
There was a problem hiding this comment.
Toast message shows provider identifier instead of display name.
Line 71 displays disconnectingProviderRef.current (e.g., "discord") instead of the user-friendly name (e.g., "Discord").
Apply this diff to show the proper provider name:
const onSubmitSuccess = (_r: unknown) => {
const username = outletContext.currentUser?.username;
+ const providerName = PROVIDERS.find(
+ p => p.identifier === disconnectingProviderRef.current
+ )?.name || disconnectingProviderRef.current;
revalidator.revalidate();
toast.addToast({
csVariant: "success",
children: (
<>
- User {username} was disconnected from{" "}
- {disconnectingProviderRef.current}
+ User {username} was disconnected from {providerName}
</>
),
duration: 30000,
});
disconnectingProviderRef.current = null;
};Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/cyberstorm-remix/app/settings/user/Connections/Connections.tsx around
lines 61-78, the toast currently shows the raw provider identifier
(disconnectingProviderRef.current) instead of a user-friendly display name;
before calling toast.addToast, resolve the display name by looking up the
provider object for that identifier (e.g., from the providers list/context or a
helper getDisplayName function), fallback to a capitalized version of the
identifier if not found, and use that resolved display name in the toast
message; keep the rest of the flow (revalidator.revalidate and clearing
disconnectingProviderRef.current) unchanged.
Uh oh!
There was an error while loading. Please reload this page.