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.

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 token expiration error loop (#6731) #8802

Merged

Conversation

eliasylonen
Copy link
Contributor

@eliasylonen eliasylonen commented Nov 28, 2024

Fixes issue #6731

Problem: After access token token expires, scrolling down the RecordTable component will put it to an infinite loop of trying to fetch records and printing errors on every iteration.

Solution: Disable fetching until component remount if a FORBIDDEN error is encountered.

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR implements error handling for token expiration in the RecordTable component to prevent infinite fetch loops when scrolling.

  • Added tableEncounteredUnrecoverableErrorComponentState to track FORBIDDEN errors in /packages/twenty-front/src/modules/object-record/record-table/states/tableEncounteredUnrecoverableErrorComponentState.ts
  • Modified fetchMoreRecords in useFetchMoreRecordsWithPagination.ts to return error information for FORBIDDEN detection
  • Added error state handling in RecordTableNoRecordGroupBodyEffect.tsx to stop fetching when FORBIDDEN errors occur
  • Implemented error detection logic to prevent further fetch attempts until component remount

💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!

3 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile

@twentyhq twentyhq deleted a comment from greptile-apps bot Nov 29, 2024
Copy link
Member

@FelixMalfait FelixMalfait left a comment

Choose a reason for hiding this comment

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

Perfect! thank you!

@FelixMalfait FelixMalfait merged commit c878f09 into twentyhq:main Nov 29, 2024
17 checks passed

const isForbidden =
result?.error?.graphQLErrors.some(
(e) => e.extensions?.code === 'FORBIDDEN',
Copy link
Member

Choose a reason for hiding this comment

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

Agree with the strat, I think we could do it for all errors (not only FORBIDDEN)
Thanks for fixing this one!

Copy link
Member

Choose a reason for hiding this comment

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

@charlesBochet I'm not sure that's a good idea. In the case of forbidden you know the user will get kicked out and things will be reset. In other cases it seems to be a trickier problem (when should we allow fetch more again?), might be best to solve it when we know for sure it causes an issue / have real-life examples

Copy link
Member

Choose a reason for hiding this comment

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

ok, LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants