Use Suspense in team setting tabs #1604
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
apps/cyberstorm-remix/app/settings/teams/team/tabs/Settings/Settings.tsx
Fixed
Show fixed
Hide fixed
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1604 +/- ##
=========================================
- Coverage 9.88% 9.85% -0.03%
=========================================
Files 310 313 +3
Lines 22489 22548 +59
Branches 406 409 +3
=========================================
Hits 2223 2223
- Misses 20266 20325 +59 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
WalkthroughThe PR refactors team settings UI components (Members, Profile, ServiceAccounts, Settings) by extracting inline form and table logic into dedicated subcomponents (MemberAddForm, MembersTable, ServiceAccountsTable, ProfileForm). It replaces the previous monolithic component structure with a Suspense/Await-based async data handling pattern. HydrateFallback components are removed throughout. The Settings loader data shape is updated to include teamName, and authentication checks are moved earlier in the render cycle. Form handling is consolidated into reusable components that manage their own state and submission flows. Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
| <NewTextInput | ||
| name={"username"} | ||
| placeholder={"Enter username..."} | ||
| onChange={(e) => { | ||
| updateFormFieldState({ | ||
| field: "username", | ||
| value: e.target.value, | ||
| }); | ||
| }} | ||
| rootClasses="add-member-form__username-input" | ||
| id="username" | ||
| /> | ||
| </div> | ||
| <div className="add-member-form__field"> | ||
| <label className="add-member-form__label" htmlFor="role"> | ||
| Role | ||
| </label> | ||
| <NewSelect | ||
| name={"role"} | ||
| options={roleOptions} | ||
| defaultValue="member" | ||
| placeholder="Select role..." | ||
| onChange={(value) => { | ||
| updateFormFieldState({ field: "role", value: value }); | ||
| }} | ||
| id="role" | ||
| /> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
Should the value prop be passed to the TextInput and NewSelect components here, so that they are controlled and properly synced with the form state?
There was a problem hiding this comment.
Or well NewSelect will work without the value set, as it's using a Radix component, that will hold the state based on the defaultValue and onChange changes.
NewTextInput does need the value
There was a problem hiding this comment.
Added for both. It was in fact the NewSelect that required the value prop to work prop-erly. NewTextInput at least seemed seemed to work without it.
| const tableData = members.map((member, index) => { | ||
| return [ | ||
| { | ||
| value: ( | ||
| <NewLink | ||
| primitiveType="cyberstormLink" | ||
| linkId="User" | ||
| key={`user_${index}`} | ||
| user={member.username} | ||
| rootClasses="members__user" | ||
| > | ||
| <NewAvatar | ||
| src={member.avatar} | ||
| username={member.username} | ||
| csSize="small" | ||
| /> | ||
| <span>{member.username}</span> | ||
| </NewLink> | ||
| ), | ||
| sortValue: member.username, | ||
| }, | ||
| { | ||
| value: ( | ||
| <div key={`role_${index}`}> | ||
| <NewSelect | ||
| csSize="xsmall" | ||
| options={roleOptions} | ||
| value={member.role} | ||
| onChange={(val: "owner" | "member") => | ||
| changeMemberRole(member.username, val) | ||
| } | ||
| /> | ||
| </div> | ||
| ), | ||
| sortValue: member.role, | ||
| }, | ||
| { | ||
| value: ( | ||
| <RemoveTeamMemberForm | ||
| indexKey={`action_${index}`} | ||
| teamName={teamName} | ||
| userName={member.username} | ||
| updateTrigger={updateTrigger} | ||
| config={config} | ||
| /> | ||
| ), | ||
| sortValue: 0, | ||
| }, | ||
| ]; | ||
| }); |
There was a problem hiding this comment.
Should the keys here be more stable instead of relying solely on key={<x>_${index}}? For example, you could use key={<x>_${member.username}_${index}} to make them both stable and unique.
There was a problem hiding this comment.
Changed as suggested.
| </p> | ||
| <LeaveTeamForm | ||
| userName={currentUser} | ||
| teamName={teamName} |
There was a problem hiding this comment.
Should this use resolvedTeamName?
There was a problem hiding this comment.
Fixed this is three spots.
| contact Mythic#0001 on the Thunderstore Discord. | ||
| </p> | ||
| <DisbandTeamForm | ||
| teamName={teamName} |
There was a problem hiding this comment.
Should this use resolvedTeamName?
| export const clientLoader = makeTeamSettingsTabLoader( | ||
| // TODO: add end point for checking can leave/disband status. | ||
| async (dapper, teamName) => ({}) | ||
| async (dapper, teamName) => ({ teamName }) | ||
| ); |
There was a problem hiding this comment.
Why is there a wrapper for this? Should the clientLoader return something?
There was a problem hiding this comment.
I'm not sure if by "wrapper" you're referring to:
- the parenthesis around
{ teamname }: that's just something the linter in the mod manager project requires and I've inadvertently carried the habit over - the fact that we have an async function that takes
teamNameas argument only to return it: that's because that's whatmakeTeamSettingsTabLoaderexpects as an argument. This is just a placeholder for now, in a later PR this will actually call the backend via Dapper. makeTeamSettingsTabLoaderitself: it's a helper function that returns a function that will actually make clientLoader to return a value. It was introduced in the preceding PR to reduce boilerplate, as all four tabs of the team settings view follow the same pattern (as shown in that PR)
There was a problem hiding this comment.
I did no code changes for this one.
Roffenlund
left a comment
Roffenlund
left a comment
There was a problem hiding this comment.
Few comments, mainly minor/unclear things.
As part of the upcoming change, clientLoader will start returning a Promise rather than the resolved value. For Suspense to work, the body of the component function can't depend on the resolved values, so move such parts into subcomponents.
As part of the upcoming change, clientLoader will start returning a Promise rather than the resolved value. For Suspense to work, the body of the component function can't depend on the resolved values, so move such parts into subcomponents.
As part of the upcoming change, clientLoader will start returning a Promise rather than the resolved value. For Suspense to work, the body of the component function can't depend on the resolved values, so move such parts into subcomponents.
Using await in clientLoader causes navigating between the tabs to do nothing until the clientLoader promise resolves. Removing the await forces the component to deal with the promises returned by the clientLoader, for which Suspense+Await components is a good pattern. Using it rendes HydarteFallback obsolete. All tabs use all-or-nothing rendering policy. For profile tab, only a form is rendered and it requires the resolved values. For Members and Service Account tabs we could consider rendering the sidebar and empty table initially, but I though it would be awkward for the add button to pop on the sidebar once the download finished (or alternatively a skeleton component to disappear). For Settings tab partial rendering could be considered, but that tab doesn't have an API endpoint available yet and shows only dummy data. On the other hand, for consistency, all-or-nothing strategy could be used there as well.
anttimaki
force-pushed
the
team-settings-pt4-suspense
branch
from
79cf5dd to
32a09a2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/cyberstorm-remix/app/settings/teams/team/tabs/Members/MembersTable.tsx (1)
74-123: Keys may be unnecessary on individual cell elements.The
keyprops on lines 81 and 97 are applied to elements that aren't direct children of an array iteration, so they may not be needed. Only theindexKeyon line 113 is actually used by the RemoveTeamMemberForm component. Consider removing the keys on lines 81 and 97 unless NewTable specifically requires them.
🧹 Nitpick comments (2)
apps/cyberstorm-remix/app/settings/teams/team/tabs/Members/Members.tsx (2)
31-31: Consider using a styled loading component.The fallback uses a plain
<div>Loading...</div>which lacks styling. Consider using a consistent loading component or spinner from your design system.
32-56: Consider adding error handling to the Await component.The
Awaitcomponent supports anerrorElementprop for handling promise rejections inline. While the loader's error handling may throw a Response that gets caught by a parent error boundary, adding explicit error handling here would provide better user experience and more granular control.Example:
- <Await resolve={members}> + <Await + resolve={members} + errorElement={<div>Failed to load members. Please try again.</div>} + >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/cyberstorm-remix/app/settings/teams/team/tabs/Members/MemberAddForm.tsx(1 hunks)apps/cyberstorm-remix/app/settings/teams/team/tabs/Members/Members.tsx(1 hunks)apps/cyberstorm-remix/app/settings/teams/team/tabs/Members/MembersTable.tsx(1 hunks)apps/cyberstorm-remix/app/settings/teams/team/tabs/Profile/Profile.tsx(3 hunks)apps/cyberstorm-remix/app/settings/teams/team/tabs/ServiceAccounts/ServiceAccounts.tsx(3 hunks)apps/cyberstorm-remix/app/settings/teams/team/tabs/ServiceAccounts/ServiceAccountsTable.tsx(1 hunks)apps/cyberstorm-remix/app/settings/teams/team/tabs/Settings/Settings.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
apps/cyberstorm-remix/app/settings/teams/team/tabs/Profile/Profile.tsx (10)
apps/cyberstorm-remix/app/settings/teams/team/tabs/Members/Members.tsx (1)
clientLoader(15-19)apps/cyberstorm-remix/app/settings/teams/team/tabs/ServiceAccounts/ServiceAccounts.tsx (1)
clientLoader(30-34)apps/cyberstorm-remix/app/p/packageListing.tsx (1)
clientLoader(121-150)apps/cyberstorm-remix/app/root.tsx (2)
clientLoader(122-173)OutletContextShape(84-89)apps/cyberstorm-remix/app/p/tabs/Required/Required.tsx (1)
clientLoader(44-76)apps/cyberstorm-remix/app/p/tabs/Versions/Versions.tsx (1)
clientLoader(44-65)apps/cyberstorm-remix/app/p/tabs/Readme/Readme.tsx (1)
clientLoader(32-50)apps/cyberstorm-remix/app/p/tabs/Changelog/Changelog.tsx (1)
clientLoader(35-56)apps/cyberstorm-remix/app/c/community.tsx (1)
clientLoader(50-65)packages/thunderstore-api/src/schemas/objectSchemas.ts (1)
TeamDetails(109-109)
apps/cyberstorm-remix/app/settings/teams/team/tabs/Settings/Settings.tsx (5)
apps/cyberstorm-remix/app/settings/teams/team/tabs/Members/Members.tsx (1)
clientLoader(15-19)apps/cyberstorm-remix/app/settings/teams/team/tabs/Profile/Profile.tsx (1)
clientLoader(21-28)apps/cyberstorm-remix/app/settings/teams/team/tabs/ServiceAccounts/ServiceAccounts.tsx (1)
clientLoader(30-34)apps/cyberstorm-remix/app/root.tsx (2)
clientLoader(122-173)OutletContextShape(84-89)packages/cyberstorm/src/newComponents/Toast/Provider.tsx (1)
useToast(79-87)
apps/cyberstorm-remix/app/settings/teams/team/tabs/ServiceAccounts/ServiceAccountsTable.tsx (4)
packages/thunderstore-api/src/schemas/objectSchemas.ts (1)
TeamServiceAccount(378-378)apps/cyberstorm-remix/app/root.tsx (1)
OutletContextShape(84-89)apps/cyberstorm-remix/app/settings/teams/team/tabs/ServiceAccounts/ServiceAccountRemoveModal.tsx (1)
ServiceAccountRemoveModal(18-104)packages/cyberstorm/src/index.ts (1)
TableSort(103-103)
apps/cyberstorm-remix/app/settings/teams/team/tabs/ServiceAccounts/ServiceAccounts.tsx (4)
apps/cyberstorm-remix/app/settings/teams/team/tabs/Members/Members.tsx (1)
clientLoader(15-19)apps/cyberstorm-remix/app/root.tsx (2)
clientLoader(122-173)OutletContextShape(84-89)apps/cyberstorm-remix/cyberstorm/utils/dapperClientLoaders.ts (1)
makeTeamSettingsTabLoader(8-29)apps/cyberstorm-remix/app/settings/teams/team/tabs/ServiceAccounts/ServiceAccountsTable.tsx (1)
ServiceAccountsTable(17-67)
apps/cyberstorm-remix/app/settings/teams/team/tabs/Members/MembersTable.tsx (3)
packages/thunderstore-api/src/index.ts (1)
RequestConfig(1-7)packages/thunderstore-api/src/patch/teamEditMember.ts (1)
teamEditMember(19-43)packages/thunderstore-api/src/delete/teamRemoveMember.ts (1)
teamRemoveMember(4-23)
apps/cyberstorm-remix/app/settings/teams/team/tabs/Members/MemberAddForm.tsx (4)
packages/thunderstore-api/src/index.ts (1)
RequestConfig(1-7)packages/thunderstore-api/src/schemas/requestSchemas.ts (1)
TeamAddMemberRequestData(574-576)packages/thunderstore-api/src/post/teamMember.ts (1)
teamAddMember(13-37)apps/cyberstorm-remix/cyberstorm/utils/StrongForm/useStrongForm.ts (1)
useStrongForm(24-169)
apps/cyberstorm-remix/app/settings/teams/team/tabs/Members/Members.tsx (4)
apps/cyberstorm-remix/app/root.tsx (2)
clientLoader(122-173)OutletContextShape(84-89)apps/cyberstorm-remix/cyberstorm/utils/dapperClientLoaders.ts (1)
makeTeamSettingsTabLoader(8-29)apps/cyberstorm-remix/app/settings/teams/team/tabs/Members/MemberAddForm.tsx (1)
MemberAddForm(27-160)apps/cyberstorm-remix/app/settings/teams/team/tabs/Members/MembersTable.tsx (1)
MembersTable(37-133)
⏰ 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). (3)
- GitHub Check: Build
- GitHub Check: Generate visual diffs
- GitHub Check: ESLint
🔇 Additional comments (6)
apps/cyberstorm-remix/app/settings/teams/team/tabs/Members/MembersTable.tsx (4)
1-35: LGTM!Imports and constant definitions are clean and appropriate.
37-73: LGTM!The ApiAction setup for role changes is well-structured with proper success/error handling and data refresh.
125-136: LGTM!Table rendering is clean with sensible default sorting.
137-212: LGTM!The RemoveTeamMemberForm correctly handles the removal flow with proper confirmation dialog, error handling, and modal state management. The modal only closes on successful removal, allowing users to retry on error.
apps/cyberstorm-remix/app/settings/teams/team/tabs/Members/Members.tsx (2)
1-19: LGTM!The loader correctly returns the members promise for deferred resolution via Await, enabling the Suspense pattern.
21-29: LGTM!Component setup is clean and the revalidation function integrates well with the new pattern.
| function AddServiceAccountForm(props: { | ||
| teamName: string; | ||
| config: () => RequestConfig; | ||
| serviceAccountRevalidate?: () => void; | ||
| serviceAccountRevalidate: () => void; | ||
| }) { | ||
| const outletContext = useOutletContext() as OutletContextShape; |
There was a problem hiding this comment.
Fix prop type for serviceAccountRevalidate
Line 77 declares serviceAccountRevalidate as () => void, but the value passed in (serviceAccountRevalidate from the parent) is an async function returning Promise<void>. TypeScript will reject this assignment (Promise<void> is not assignable to void), so the build breaks. Update the prop type to accept the promised result (and keep the call site unchanged).
function AddServiceAccountForm(props: {
teamName: string;
- serviceAccountRevalidate: () => void;
+ serviceAccountRevalidate: () => Promise<void>;
}) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function AddServiceAccountForm(props: { | |
| teamName: string; | |
| config: () => RequestConfig; | |
| serviceAccountRevalidate?: () => void; | |
| serviceAccountRevalidate: () => void; | |
| }) { | |
| const outletContext = useOutletContext() as OutletContextShape; | |
| function AddServiceAccountForm(props: { | |
| teamName: string; | |
| serviceAccountRevalidate: () => Promise<void>; | |
| }) { | |
| const outletContext = useOutletContext() as OutletContextShape; |
🤖 Prompt for AI Agents
In
apps/cyberstorm-remix/app/settings/teams/team/tabs/ServiceAccounts/ServiceAccounts.tsx
around lines 75 to 79, the AddServiceAccountForm prop serviceAccountRevalidate
is typed as () => void but the parent passes an async function returning
Promise<void>; update the prop type to () => Promise<void> so the types align
and the build succeeds, leaving the call site unchanged.
No description provided.