-
-
Notifications
You must be signed in to change notification settings - Fork 232
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: fixed crash on list contributor grid when in card view changing page size #2241
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,10 +2,11 @@ import { GetServerSidePropsContext } from "next"; | |
import { createPagesServerClient } from "@supabase/auth-helpers-nextjs"; | ||
import { useRouter } from "next/router"; | ||
|
||
import { ErrorBoundary } from "react-error-boundary"; | ||
import ListPageLayout from "layouts/lists"; | ||
import { fetchApiData, validateListPath } from "helpers/fetchApiData"; | ||
import Error from "components/atoms/Error/Error"; | ||
import { convertToContributors, useContributorsList } from "lib/hooks/api/useContributorList"; | ||
import { useContributorsList } from "lib/hooks/api/useContributorList"; | ||
import ContributorsList from "components/organisms/ContributorsList/contributors-list"; | ||
import { FilterParams } from "./activity"; | ||
|
||
|
@@ -17,48 +18,38 @@ export const getServerSideProps = async (ctx: GetServerSidePropsContext) => { | |
} = await supabase.auth.getSession(); | ||
const bearerToken = session ? session.access_token : ""; | ||
|
||
const { listId, limit: rawLimit, range } = ctx.params as FilterParams; | ||
const { listId } = ctx.params as FilterParams; | ||
|
||
const limit = rawLimit ?? "10"; // Can pull this from the querystring in the future | ||
const [{ data, error: contributorListError }, { data: list, error }] = await Promise.all([ | ||
fetchApiData<PagedData<DBListContributor>>({ | ||
path: `lists/${listId}/contributors?limit=${limit}&range=${range ?? "30"}`, | ||
bearerToken, | ||
pathValidator: validateListPath, | ||
}), | ||
fetchApiData<DBList>({ path: `lists/${listId}`, bearerToken, pathValidator: validateListPath }), | ||
]); | ||
const { data: list, error } = await fetchApiData<DBList>({ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've removed server-side rendering contributor list data and only kept critical stuff to load the page, e.g. list info. Something related to the initial data for the contributor list is what caused the issue with the card view. Still trying to work out why, but regardless it makes sense to only load this client-side going forward as mentioned in the PR description. |
||
path: `lists/${listId}`, | ||
bearerToken, | ||
pathValidator: validateListPath, | ||
}); | ||
|
||
if (error?.status === 404) { | ||
return { | ||
notFound: true, | ||
}; | ||
} | ||
|
||
const contributors = convertToContributors(data?.data); | ||
const userId = Number(session?.user.user_metadata.sub); | ||
|
||
return { | ||
props: { | ||
list, | ||
initialData: data ? { data: contributors, meta: data.meta } : { data: [], meta: {} }, | ||
isError: error || contributorListError, | ||
isError: error, | ||
isOwner: list && list.user_id === userId, | ||
}, | ||
}; | ||
}; | ||
|
||
interface ContributorListPageProps { | ||
list?: DBList; | ||
initialData: { | ||
meta: Meta; | ||
data: DbPRContributor[]; | ||
}; | ||
isError: boolean; | ||
isOwner: boolean; | ||
} | ||
|
||
const ContributorsListPage = ({ list, initialData, isError, isOwner }: ContributorListPageProps) => { | ||
const ContributorsListPage = ({ list, isError, isOwner }: ContributorListPageProps) => { | ||
const router = useRouter(); | ||
const { range, limit } = router.query; | ||
|
||
|
@@ -68,7 +59,6 @@ const ContributorsListPage = ({ list, initialData, isError, isOwner }: Contribut | |
data: { data: contributors, meta }, | ||
} = useContributorsList({ | ||
listId: list?.id, | ||
initialData, | ||
defaultRange: range ? (range as string) : "30", | ||
defaultLimit: limit ? (limit as unknown as number) : 10, | ||
}); | ||
|
@@ -78,13 +68,17 @@ const ContributorsListPage = ({ list, initialData, isError, isOwner }: Contribut | |
{isError ? ( | ||
<Error errorMessage="Unable to load list of contributors" /> | ||
) : ( | ||
<ContributorsList | ||
contributors={contributors} | ||
meta={meta} | ||
isLoading={isLoading} | ||
setPage={setPage} | ||
range={String(range) ?? "30"} | ||
/> | ||
<ErrorBoundary | ||
fallback={<div className="grid place-content-center">Error loading the list of contributors</div>} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I went with a barebones fallback error message @isabensusan, but going forward a bit of styling for this would be nice as we can use this for graphs or other data driven components. It can be in another PR though. |
||
> | ||
<ContributorsList | ||
contributors={contributors} | ||
meta={meta} | ||
isLoading={isLoading} | ||
setPage={setPage} | ||
range={String(range) ?? "30"} | ||
/> | ||
</ErrorBoundary> | ||
)} | ||
</ListPageLayout> | ||
); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
react-error-boundary will allow us to have a fallback view if we wrap a component with it that crashes.