Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix: issue 4485, check 4484 #4508

Merged
merged 2 commits into from
Aug 31, 2023
Merged

Fix: issue 4485, check 4484 #4508

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
21abeb6
Fix 4485, 4484: update the code again
mkbeefcake Aug 31, 2023
324f845
Fix 4485, 4484: Lint issue and ui-unit test
mkbeefcake Aug 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 1 addition & 11 deletions packages/ui/src/app/pages/Forum/ForumCategory.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,7 @@ import { PageTitle } from '@/common/components/page/PageTitle'
import { PreviousPage } from '@/common/components/page/PreviousPage'
import { Label, TextMedium } from '@/common/components/typography'
import { useModal } from '@/common/hooks/useModal'
import { useRefetchQueries } from '@/common/hooks/useRefetchQueries'
import { useSort } from '@/common/hooks/useSort'
import { MILLISECONDS_PER_BLOCK } from '@/common/model/formatters'
import { ForumCategoryList } from '@/forum/components/category/ForumCategoryList'
import { ForumPageHeader } from '@/forum/components/ForumPageHeader'
import { ThreadFilters } from '@/forum/components/threads/ThreadFilters'
Expand Down Expand Up @@ -48,17 +46,9 @@ export const ForumCategory = () => {
},
{ perPage: THREADS_PER_PAGE, page }
)
const isRefetched = useRefetchQueries({
interval: MILLISECONDS_PER_BLOCK,
include: ['GetForumThreads', 'GetForumThreadsCount'],
})

const { showModal } = useModal()

if (isLoadingThreads && !isRefetched) {
return <Loading />
}

Comment on lines -58 to -61
Copy link
Member

Choose a reason for hiding this comment

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

Great work on catching this ! It wasn't an easy one to debug 🤯

But I think the fix could be improved because first it now makes empty categories show a loader every 6 seconds e.g: https://dao-git-fork-mkbeefcake-fix-4485-4484-joystream.vercel.app/#/forum/category/17?network-config=https://34.230.5.182.nip.io/network/config.json. But more importantly it still refetch way to much data (as you pointed out on #4484). Also I realized we've struggled with this particular useRefetch for a long time (see #4318) so I really think the best is to remove this one.

Instead we could replace this logic by a pollInterval of 6 second in packages/ui/src/forum/hooks/useForumCategoryThreads.ts but only when isArchive is false. Also isLoading should become !threadsData || !countData this way isLoading is only true on the initial load.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image
Thanks for your guide. also I figured out "posts" field in forumThreads query, If i choose this mechanism, i can reduce the query count to 1, but it could take all posts info per each thread, so could take much data and a bit more loading time.
I hope to get your opinion about this.

Copy link
Member

Choose a reason for hiding this comment

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

Yes forumThreads { posts { ... } } will return all the posts for all the threads shown one the category page so I think 1 request per thread is a better trade off.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@thesan I have added posts field for ForumThread interface and fetched it in GetForumThreads Query, so reduce the queries in Forum Category page, please check the updated code.

if (isLoadingCategory) {
return <Loading />
}
Expand Down Expand Up @@ -126,7 +116,7 @@ export const ForumCategory = () => {
<ThreadList
threads={threads}
getSortProps={getSortProps}
isLoading={isLoadingThreads && !isRefetched}
isLoading={isLoadingThreads}
isArchive={isArchive}
page={page}
pageCount={threadCount && Math.ceil(threadCount / THREADS_PER_PAGE)}
Expand Down
9 changes: 6 additions & 3 deletions packages/ui/src/forum/hooks/useForumCategoryThreads.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { Reducer, useEffect, useMemo, useReducer } from 'react'

import { ForumThreadOrderByInput } from '@/common/api/queries'
import { SortOrder, toQueryOrderByInput } from '@/common/hooks/useSort'
import { MILLISECONDS_PER_BLOCK } from '@/common/model/formatters'
import { merge } from '@/common/utils'
import { ThreadEmptyFilters, ThreadFiltersState } from '@/forum/components/threads/ThreadFilters'
import { useGetForumThreadsCountQuery, useGetForumThreadsQuery } from '@/forum/queries'
Expand Down Expand Up @@ -31,7 +32,7 @@ export const useForumCategoryThreads = (

const [{ filters, categoryId, isArchive }, refresh] = useReducer(threadOptionReducer, initialOptions)

const { loading: loadingThreads, data: threadsData } = useGetForumThreadsQuery({
const { data: threadsData } = useGetForumThreadsQuery({
variables: {
where: where(filters, categoryId, isArchive),
orderBy: [ForumThreadOrderByInput.IsStickyDesc, toQueryOrderByInput<ForumThreadOrderByInput>(options.order)],
Expand All @@ -42,15 +43,17 @@ export const useForumCategoryThreads = (
offset: (pagination.page - 1) * pagination.perPage,
}),
},
pollInterval: isArchive === false ? MILLISECONDS_PER_BLOCK : 0,
})

const { loading: loadingCount, data: countData } = useGetForumThreadsCountQuery({
const { data: countData } = useGetForumThreadsCountQuery({
variables: { where: where(filters, categoryId, isArchive) },
pollInterval: isArchive === false ? MILLISECONDS_PER_BLOCK : 0,
})
const totalCount = countData?.forumThreadsConnection.totalCount

return {
isLoading: loadingThreads || loadingCount,
isLoading: !threadsData || !countData,
threads: threadsData?.forumThreads.map((thread) => asForumThread(thread)) ?? [],
threadCount: totalCount,
refresh,
Expand Down
7 changes: 7 additions & 0 deletions packages/ui/test/forum/hooks/useForumCategoryThreads.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ describe('useForumCategoryThreads', () => {
orderBy: [IsStickyDesc, UpdatedAtDesc],
limit: 30,
},
pollInterval: 0,
})
})

Expand All @@ -64,6 +65,7 @@ describe('useForumCategoryThreads', () => {
orderBy: [IsStickyDesc, UpdatedAtDesc],
limit: 30,
},
pollInterval: 0,
})

act(() => refresh({ filters: { author, date: { start, end }, tag: null } }))
Expand All @@ -82,6 +84,7 @@ describe('useForumCategoryThreads', () => {
orderBy: [IsStickyDesc, UpdatedAtDesc],
limit: 30,
},
pollInterval: 0,
})
})

Expand All @@ -94,6 +97,7 @@ describe('useForumCategoryThreads', () => {
orderBy: [IsStickyDesc, AuthorDesc],
limit: 30,
},
pollInterval: 0,
})
})

Expand All @@ -111,6 +115,7 @@ describe('useForumCategoryThreads', () => {
orderBy: [IsStickyDesc, UpdatedAtDesc],
limit: 30,
},
pollInterval: 0,
})

act(() => rerender([{ isArchive: true, order, filters: { author: null, date: { start, end }, tag: null } }]))
Expand All @@ -126,6 +131,7 @@ describe('useForumCategoryThreads', () => {
orderBy: [IsStickyDesc, UpdatedAtDesc],
limit: 30,
},
pollInterval: 0,
})
})

Expand All @@ -139,6 +145,7 @@ describe('useForumCategoryThreads', () => {
limit: 10,
offset: 0,
},
pollInterval: 0,
})
})
})