fix: updated record calculations to display the correct record index #16231
Conversation
… total number of records
Welcome!
Hello there, congrats on your first PR! We're excited to have you contributing to this project. |
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
| const { records: allRecords } = useFindManyRecords({ | ||
| filter, | ||
| orderBy, | ||
| objectNameSingular, | ||
| recordGqlFields: { id: true }, | ||
| }); |
There was a problem hiding this comment.
Bug: useFindManyRecords defaults to 60 records, causing incorrect rank display for records beyond this limit.
Severity: CRITICAL | Confidence: High
🔍 Detailed Analysis
The useFindManyRecords hook is called without a limit parameter, causing it to default to QUERY_DEFAULT_LIMIT_RECORDS = 60. This results in allRecords containing only up to 60 records instead of all records. If a record exists beyond the 60th position in the filtered view, allRecords.findIndex() will return -1, leading to an incorrect display of the record's rank, such as "-1 of {totalCount}" or "(0/{totalCount})" in the UI.
💡 Suggested Fix
Explicitly pass limit: -1 (or a sufficiently large number) to useFindManyRecords to ensure all records are fetched for accurate rank calculation.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location:
packages/twenty-front/src/modules/object-record/record-show/hooks/useRecordShowPagePagination.ts#L215-L220
Potential issue: The `useFindManyRecords` hook is called without a `limit` parameter,
causing it to default to `QUERY_DEFAULT_LIMIT_RECORDS = 60`. This results in
`allRecords` containing only up to 60 records instead of all records. If a record exists
beyond the 60th position in the filtered view, `allRecords.findIndex()` will return
`-1`, leading to an incorrect display of the record's rank, such as `"-1 of
{totalCount}"` or `"(0/{totalCount})"` in the UI.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 4658569
Greptile OverviewGreptile SummaryReplaced cache-based rank calculation with a new
The approach is directionally correct (cache was unreliable), but the implementation has off-by-one error and won't scale beyond 60 records. Confidence Score: 1/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant User
participant Hook as useRecordShowPagePagination
participant API as useFindManyRecords
participant Cache as Apollo Cache
User->>Hook: Open record details
Note over Hook: Get current record cursor
Hook->>API: Fetch current record (limit: 1)
API-->>Hook: Current record + cursor
par Fetch surrounding records
Hook->>API: Fetch records before cursor (limit: 1)
API-->>Hook: Record before + totalCountBefore
and
Hook->>API: Fetch records after cursor (limit: 1)
API-->>Hook: Record after + totalCountAfter
end
Note over Hook: NEW: Fetch all records for rank
Hook->>API: Fetch all records (default limit: 60)
API-->>Hook: allRecords array
Note over Hook: Calculate rank using findIndex()
Hook->>Hook: rankInView = allRecords.findIndex(...)
Hook->>Hook: Calculate totalCount from before/after
Hook-->>User: Display "{rankInView} of {totalCount}"
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| }); | ||
|
|
||
| const rankFoundInView = rankInView > -1; | ||
| const rankInView = allRecords.findIndex((record) => record.id === objectRecordId); |
There was a problem hiding this comment.
logic: findIndex returns -1 when the record is not found. This will display "-1 of X" when the record is not in the loaded results (which can happen since allRecords only fetches 60 records by default). Add a check:
| const rankInView = allRecords.findIndex((record) => record.id === objectRecordId); | |
| const rankInView = allRecords.findIndex((record) => record.id === objectRecordId) + 1; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/twenty-front/src/modules/object-record/record-show/hooks/useRecordShowPagePagination.ts
Line: 222:222
Comment:
**logic:** `findIndex` returns -1 when the record is not found. This will display "-1 of X" when the record is not in the loaded results (which can happen since `allRecords` only fetches 60 records by default). Add a check:
```suggestion
const rankInView = allRecords.findIndex((record) => record.id === objectRecordId) + 1;
```
How can I resolve this? If you propose a fix, please make it concise.| const { records: allRecords } = useFindManyRecords({ | ||
| filter, | ||
| orderBy, | ||
| objectNameSingular, | ||
| recordGqlFields: { id: true }, | ||
| }); |
There was a problem hiding this comment.
logic: Fetching all records without a limit will cause performance issues with large datasets. The default limit is 60, but if there are more records than this, the rankInView calculation will be incorrect. Consider either:
- Using a higher limit (e.g., the max authorized by backend)
- Using the cursor-based pagination logic that's already in place
- Passing
limit: undefinedto fetch all records (if backend supports it)
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/twenty-front/src/modules/object-record/record-show/hooks/useRecordShowPagePagination.ts
Line: 215:220
Comment:
**logic:** Fetching all records without a limit will cause performance issues with large datasets. The default limit is 60, but if there are more records than this, the `rankInView` calculation will be incorrect. Consider either:
1. Using a higher limit (e.g., the max authorized by backend)
2. Using the cursor-based pagination logic that's already in place
3. Passing `limit: undefined` to fetch all records (if backend supports it)
How can I resolve this? If you propose a fix, please make it concise.
|
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:11893 This environment will automatically shut down when the PR is closed or after 5 hours. |
3 participants
Issue Number: #11977
Context:
As shown in the issue description, the record details for each person were incorrect. For each person, no matter their record index, it would display
0 / (number of all records). The intended behavior is to display(record index) / (number of all records). This logic should extend to the deleted records page as well.Changes Made:
The rankInView was previously being calculated using the cache but this was constantly returning -1. I updated the calculation to query all the records matching the filter and then using
findIndex()to find the rank in the view.Correct Output:
output.mov
output2.mov