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

Fixes person navigation after image upload #9076

Merged
merged 2 commits into from
Dec 18, 2024
Merged

Fixes person navigation after image upload #9076

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
17c0abc
Fixes person navigation after image upload
mdrazak2001 Dec 15, 2024
107f4e9
preserve view filters
mdrazak2001 Dec 18, 2024
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
8 changes: 6 additions & 2 deletions ...s/twenty-front/src/modules/object-record/record-show/hooks/useRecordShowPagePagination.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,9 @@ export const useRecordShowPagePagination = (
useFindManyRecords({
skip: loadingCursor,
fetchPolicy: 'network-only',
filter,
filter: {
id: { neq: objectRecordId },
},
orderBy,
cursorFilter: isNonEmptyString(cursorFromRequest)
? {
Expand All @@ -81,7 +83,9 @@ export const useRecordShowPagePagination = (
const { loading: loadingRecordAfter, records: recordsAfter } =
useFindManyRecords({
skip: loadingCursor,
filter,
filter: {
id: { neq: objectRecordId },
Copy link
Member

Choose a reason for hiding this comment

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

this does not feels right to me, we need to retrieve the previous (resp. next) record based on the viewFilters and sort. We cannot remove the filter as you are doing.

Could you explain your change and how you think it's fixing the issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @charlesBochet,

Thanks for the feedback! You're right—replacing filter entirely was incorrect. I've updated the code to combine the original filter with { id: { neq: objectRecordId } } to ensure we respect view filters and sorting while excluding the current record (to prevent duplicate pagination results).

Let me know if this works!

Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit confused because to me this should not be tied to filters.
It should be tied to cursor / pagination logic. To get the next record for example, we do:

cursorFilter: cursorFromRequest
        ? {
            cursorDirection: 'after',
            cursor: cursorFromRequest,
            limit: 1,
          }
        : undefined,

This should already get the next item without having to change the filters. Why isn't this working?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did some logging to understand the behavior further. Here’s what I observed:

  1. Before the image upload (Added Dummy ids for the below example):
  •     Records Before: []

  •     Records After (contains the next person information):
      [
        {
          "__typename": "Person",
          "id": "B"
        }
      ]
  1. After the image upload:
  •     Records Before:
       [
          {
            "__typename": "Person",
            "id": "A"
          }
        ]

  •     Records After :
       [
          {
            "__typename": "Person",
            "id": "A"
          },
          {
            "__typename": "Person",
            "id": "B"
          }
        ]

These responses were returned by useFindManyRecords. It seems that after the image upload, the Records After array includes both the current record and the next record. As a result, when the "Next" button is pressed, it navigates to the current person instead of the expected next person.
This behavior suggests that while the cursor logic retrieves records after the current position, the filtering isn't excluding the current record
Explicitly adding a filter like { id: { neq: currentRecordId } } ensures the current record is excluded, preventing this duplication and aligning the navigation behavior with expectations.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, I think this is an optimistic rendering issue. What's likely happening is that on record update (useUpdateOneRecord) we are triggering optimistic effects triggerUpdateRecordOptimisticEffect.

This Optimistic effects (through cache.modify) is checking if the updated record match the query and if yes it adds it.

  const updatedRecordShouldBeAddedToRootQueryEdges =
          updatedRecordMatchesThisRootQueryFilter &&
          !updatedRecordFoundInRootQueryEdges;

However this logic does not take the cursor into account : it's not because it matches the filter that the record should be added to the query result, it should also check that if we have orderBy + cursor + limit, it should be added.

This is not a big change but might be a bit tricky to implement.

Now I understand your change and even if not completely right it might solve the issue for now

Copy link
Member

Choose a reason for hiding this comment

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

And I think it works

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @charlesBochet for the detailed explanation, am able to connect the dots now!

},
fetchPolicy: 'network-only',
orderBy,
cursorFilter: cursorFromRequest
Expand Down
Loading