empiricaly / empirica

Open source project to tackle the problem of long development cycles required to produce software to conduct multi-participant and real-time human experiments online.
https://empirica.ly/
Apache License 2.0
45 stars 8 forks source link

exitSteps({game}) can have undefined value for game #525

Closed JamesPHoughton closed 6 months ago

JamesPHoughton commented 6 months ago

Is there an existing issue for this?

What happened?

Sentry error boundary is showing that exitSteps can be called without a valid game object. This may be (?) because the game hook (wherever it is being called) hasn't loaded properly before exitSteps is called. image

I'm not sure how these functions work - is exitSteps called as part of the render cycle, so that I could solve the problem with something like the following?

function exitSteps({ game }) {
    if (!game) return;
    ...
    const treatment = game.get("treatment");
    ...
}

Steps To Reproduce

Happens very rarely

Empirica Version

"build": {
        "version": "v1.9.0",
        "sha": "6dc8adb",
        "build": "166",
        "tag": "v1.9.0",
        "branch": "main",
        "time": "2024-01-01T08:11:38Z"
    },

What OS are you seeing the problem on?

Linux

What browser are you seeing the problem on?

Chrome

Relevant log output

No response

Anything else?

No response

Code of Conduct

npaton commented 6 months ago

Game is actually not available in exit steps. It might work sometimes because of timing. But we unload the game when the game ends.

Maybe we shouldn't unload the game. I'm open to suggestions here. The reason for removing the game was that the user shouldn't have access to it after they finished playing. They have access to that page for a long time, and could go inspect the game data, which might not be something we want. But is that really an issue…

Also, we need to fix the documentation if it says we can use the Game exit steps, and decide to keep unloading it.

This has been a problem in the past. So far, the fix I've recommended is to copy any info you need for the exit steps into the player, which remains available in the exit steps.

JamesPHoughton commented 6 months ago

That's very curious, because this code does work 99% of the time. If the intention is that game should not be available in exitSteps, I'm not sure why I am usually able to access it there.

The concern that the player can inspect some private data from other players using the game object is reasonable, if they know what to look for. I guess the primary concern is that there might be some identifying information saved in the game or players objects that we want to protect. What if we did some more minifcation/obfuscation in the production build, to make it harder for someone to figure out how to access those objects? Would that mitigate some of that concern? (I'm assuming that using sourcemaps would still be able to reconstruct the original code for tools like Sentry?).

I'd advocate for keeping access to game and players in exitSteps, because a reasonable thing to do in an exit stage would be to ask about what happened during the game itself, and what players think about it (e.g. "player X did Y during the game, please give your feedback", etc.).

frasalvi commented 6 months ago

+1 for keeping game and players in exitSteps.

In my use-case, I also ask for something with a premise along the lines of "player X did Y during the game", and for a complex game saving all the info in every player object could blow up a bit the data size. By the way, my code also makes use of game and players in exitSteps already, and only rarely fails as in this issue.

npaton commented 6 months ago

Thank you for the feedback! I created an issue to keep the game after the game end. The only concern I have is when putting the player into a new game, but I think that case is already covered, so it shouldn't be too hard to change. I will look into it as soon as I get a chance. https://github.com/empiricaly/empirica/issues/528

I think the game being available in the exit steps is due to the fact it's a callback that is only called once and then the result is "memoized". So re-renders of the UI do not call that callback again. By the time the game unloads, the exit steps callback has already run. You should be able to reproduce it by simply reloading the page after the game has exited.

frasalvi commented 6 months ago

The only concern I have is when putting the player into a new game, but I think that case is already covered, so it shouldn't be too hard to change.

I know this isn't the right issue for that, but since you mentioned that: have you tried testing whether reassigning players to new games works in recent versions? I never managed to get it to work through the "clear" button (#418), so I use my own logic

npaton commented 6 months ago

I was wrong, the game is not unloaded as I thought it was. My assumption now is that the game is not actually always loaded yet (after a refresh) when the exit steps callback is called. I have added a check for the game being loaded before the exit steps. It's hard to reproduce, since it's a load timing issue, please let me know if you still encounter the issue after the last change.

@frasalvi I'll create another issue for that. 🙏