Skip to content

using ref to track the download state change instead of a state variable #8270

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

Closed

Conversation

parmarKaran01
Copy link
Contributor

PR Description

Fix for the issue : #8239
This issue was not specific to Kanban view, it was happening for me for all the views and all the tables, but only when i fresh login and try to export

Steps to reproduce

  1. Clear the application data
  2. Login
  3. go to any table and try to export ( for the first time it will get downloaded twice, but the second time when you click on download its working as expected )

Issue

This PR addresses a bug where downloads were being triggered twice when the download button was initially clicked. The redundant triggering stemmed from asynchronous state changes (isDownloading, loading, and inflight) that caused the useEffect hook to execute multiple times unintentionally.
noticed that the isDownloading state variable is not setting correctly that was the root cause for the file getting downloaded twice
line number 122 in useRecordData hook this condition was getting false if (!isDownloading || inflight || loading) twice because of the isDownloading state not getting reset correctly in the complete function ( possibly its a stale closure )

Solution

To resolve this, a ref downloadingRef was introduced to act as a controller for the download process:

  1. Checking downloadingRef.current before initiating a new download, thereby avoiding duplicate downloads.
  2. Resetting downloadingRef.current only after the download completes.

Thank you for considering this contribution! I look forward to your feedback.

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 fixes duplicate downloads by replacing the download state tracking mechanism in useRecordData from useState to useRef, ensuring more reliable download state management.

  • Replaced isDownloading state with isDownloadingRef in /packages/twenty-front/src/modules/object-record/record-index/options/hooks/useRecordData.ts to prevent stale closures
  • Added guard clause if (!isDownloadingRef.current || inflight || loading) to prevent multiple download triggers
  • Modified download completion logic to properly reset isDownloadingRef.current after download finishes
  • Updated useEffect dependencies to include isDownloadingRef.current for proper state tracking

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

@@ -171,16 +170,19 @@ export const useRecordData = ({
previousRecordCount,
hiddenKanbanFieldColumn,
viewType,
isDownloadingRef.current,
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: isDownloadingRef.current should not be in the useEffect dependencies array since it's a mutable ref value

@thomtrp thomtrp self-assigned this Nov 5, 2024
Copy link
Contributor

@thomtrp thomtrp left a comment

Choose a reason for hiding this comment

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

Hi! We do not use useRef as a state management. You should see a linter exception. Could you please find another way to fix this? 🙏

@lucasbordeau
Copy link
Contributor

lucasbordeau commented Nov 7, 2024

@parmarKaran01 Thank you for your contribution. We chose not to use useRef for state management and created an eslint rule for that.

The problem you're trying to solve needs a refactor of this hook so that it exports a function that can be called imperatively.

I'm closing this PR because we have a lot of PR to handle and we try to avoid keeping WIP as open PR, feel free to open a new one if you want to refactor but it's not an easy task.

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

Successfully merging this pull request may close these issues.

3 participants