-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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 total count in show page #6462
Conversation
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.
PR Summary
The changes fix the total count display on the show page by separately tracking records before and after the cursor and combining these counts.
- Modified
packages/twenty-front/src/modules/object-record/record-show/hooks/useRecordShowPagePagination.ts
to track and combinetotalCountBefore
andtotalCountAfter
. - Introduced logic to handle edge cases where total counts might be incorrectly combined.
- Ensured accurate total count display even when the count is 1.
Potential issue: The logic for combining counts might still lead to inaccuracies in some edge cases.
1 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings
const totalCount = | ||
totalCountBefore === 0 | ||
? totalCountAfter === 0 | ||
? 1 | ||
: totalCountAfter | ||
: totalCountBefore; |
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.
style: Combining totalCountBefore and totalCountAfter using this logic might lead to incorrect counts if both are non-zero but not equal. Consider summing the counts instead.
@charlesBochet Can help with a review on this? |
9642211
to
ae8b44e
Compare
if (totalCountBefore !== 0 && totalCountAfter !== 0) { | ||
totalCount = Math.max(totalCountBefore, totalCountAfter); | ||
} else { | ||
totalCount = |
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.
totalCount = (totalCountBefore === 0 && totalCountAfter === 0) ? 1 : Math.max(totalCountBefore, totalCountAfter)
I believe this should work?
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.
Yup. That's a better way to write it. Let me correct this.
ae8b44e
to
e9ba016
Compare
@charlesBochet Ready for another round of review. |
@@ -147,6 +148,11 @@ export const useRecordShowPagePagination = ( | |||
|
|||
const objectLabel = capitalize(objectMetadataItem.namePlural); | |||
|
|||
const totalCount = | |||
totalCountBefore === 0 && totalCountAfter === 0 |
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.
Could be even shorter
const totalCount = Math.max(1, totalCountBefore, totalCountAfter)
e9ba016
to
2307a89
Compare
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.
LGTM!
Fixes #6405 We need to take into account both the totalCount of **before cursor** results and **after cursor** results.
Fixes #6405
We need to take into account both the totalCount of before cursor results and after cursor results.