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

Get Processing Status from Redis into the frontend #10297

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

FinnIckler
Copy link
Member

I wouldn't mind also getting the registration into the page and conditionally checking for if processing only if no registration exists, but I would need to touch the v1
@registration = @competition.registrations.find_or_initialize_by(user_id: current_user.id, competition_id: @competition.id)
line which sounds dangerous for the little time we have to get this out

@FinnIckler
Copy link
Member Author

@thewca-bot deploy staging

@FinnIckler
Copy link
Member Author

@thewca-bot deploy staging

@FinnIckler
Copy link
Member Author

FinnIckler commented Nov 22, 2024

@kr-matthews any idea why this causes an infinite loop?
-> Setting isProcessing to true from Rails
-> starting Processing polling which sets the state to true
-> that finishes and sets the state to false
-> rerendering sets the state to true again (from the boolean coming from rails presumably)
-> loop

I thought doing it like this

const [processing, setProcessing] = useState(false);

  useEffect(() => {
    setProcessing(isProcessing);
  }, [isProcessing]);

will make sure the value will not override it again.

@@ -79,6 +80,10 @@ export default function CompetingStep({

const [processing, setProcessing] = useState(false);
Copy link
Member

@gregorbg gregorbg Nov 22, 2024

Choose a reason for hiding this comment

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

What goes against just using the isProcessing that is passed in from the backend as initial value for this state?

Copy link
Member Author

Choose a reason for hiding this comment

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

When a user refreshes while processing, then processing finishes, react rerenders and it will set it to true again, which will cause it to be stuck in a process -> finish -> process loop

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried to fix that with the useEffect, but it still occurs

Copy link
Member

Choose a reason for hiding this comment

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

But the loop is being caused by the useEffect. My point was to remove the useEffect and instead use the initialization value here.

Copy link
Member Author

Choose a reason for hiding this comment

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

it is not. I had it exactly like you proposed previously and it caused the same loop

Copy link
Member Author

Choose a reason for hiding this comment

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

fc0048e
see first commit

@gregorbg
Copy link
Member

I don't see an "easy" solution here. The problem is that the "backend" version of isProcessing is taken as a ground truth, and once the polling actually finds out that the processing is done, this statically rendered flag from the backend is suddenly "out of date", meaning you'd have to ignore it completely once onProcessingComplete fired.

The technical problem is that within the body of onProcessingComplete, you're reloading the live-queried version of the registration, which lives much higher in the DOM tree and thus causes a full re-render. The re-render then tries to take this "outdated" backend information back into account.

I can see two solutions, with neither of them being overly amazing:

  1. Introducing another "tripwire" style state that gets set to true once the onProcessingComplete hook actually fired, and when that tripwire state is true, the isProcessing flag from the backend gets fully ignored. Basically a short-circuiting mechanism. Nasty hack but works.
  2. Moving the current, frontend-only processing state (the one created by the useState hook) up several levels to the same place where the registration is queried. That would mean you have to pass the setProcessing setter down several levels to be able to use it in the onProcessingComplete hook, which is also nasty. But at least it would avoid the re-render logic and you could then indeed just initialize useState with the backend value as indicated above.

The ideal solution (which requires significant effort though) would of course be to move everything into a shared useContext state that can be pulled in on-demand, completely making these render dependencies obsolete.

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.

3 participants