search

deathandmayhem / jolly-roger

Dead men tell no tales!
MIT License
18 stars 5 forks source link

Deleting a hunt causes errors loading pages #2100

Closed jpd236 closed 2 months ago

jpd236 commented 2 months ago

I'm not sure if this is entirely sufficient, but I just did the following:

And started hitting errors and getting a blank page:

model-helpers.ts:14 Uncaught TypeError: Cannot read properties of undefined (reading 'submitTemplate')
    at guessURL (model-helpers.ts:14:13)
    at NotificationCenter.tsx:300:25
    at renderWithHooks (modules.js?hash=28a15b5a2b97e8a60d0a6cf67d655d1fde40931e:78616:18)
    at updateFunctionComponent (modules.js?hash=28a15b5a2b97e8a60d0a6cf67d655d1fde40931e:81899:20)
    at updateSimpleMemoComponent (modules.js?hash=28a15b5a2b97e8a60d0a6cf67d655d1fde40931e:81736:10)
    at updateMemoComponent (modules.js?hash=28a15b5a2b97e8a60d0a6cf67d655d1fde40931e:81595:14)
    at beginWork (modules.js?hash=28a15b5a2b97e8a60d0a6cf67d655d1fde40931e:83984:16)
    at HTMLUnknownElement.callCallback (modules.js?hash=28a15b5a2b97e8a60d0a6cf67d655d1fde40931e:66475:14)
    at Object.invokeGuardedCallbackDev (modules.js?hash=28a15b5a2b97e8a60d0a6cf67d655d1fde40931e:66524:16)
    at invokeGuardedCallback (modules.js?hash=28a15b5a2b97e8a60d0a6cf67d655d1fde40931e:66588:31)

modules.js?hash=28a15b5a2b97e8a60d0a6cf67d655d1fde40931e:80998 The above error occurred in one of your React components:

    at http://localhost:3000/app/app.js?hash=f7d2a81b46608bf925e7aa996f780f27aaa1dc45:12590:7
    at div
    at http://localhost:3000/packages/modules.js?hash=28a15b5a2b97e8a60d0a6cf67d655d1fde40931e:107957:3
    at O (http://localhost:3000/packages/modules.js?hash=28a15b5a2b97e8a60d0a6cf67d655d1fde40931e:6272:23021)
    at NotificationCenter (http://localhost:3000/app/app.js?hash=f7d2a81b46608bf925e7aa996f780f27aaa1dc45:13016:34)
    at div
    at App (http://localhost:3000/app/app.js?hash=f7d2a81b46608bf925e7aa996f780f27aaa1dc45:5887:7)
    at AuthenticatedPage (http://localhost:3000/app/app.js?hash=f7d2a81b46608bf925e7aa996f780f27aaa1dc45:21727:7)
    at RenderedRoute (http://localhost:3000/packages/modules.js?hash=28a15b5a2b97e8a60d0a6cf67d655d1fde40931e:94884:5)
    at Suspense
    at BreadcrumbsProvider (http://localhost:3000/app/app.js?hash=f7d2a81b46608bf925e7aa996f780f27aaa1dc45:22027:7)
    at http://localhost:3000/app/app.js?hash=f7d2a81b46608bf925e7aa996f780f27aaa1dc45:19864:5
    at Router (http://localhost:3000/packages/modules.js?hash=28a15b5a2b97e8a60d0a6cf67d655d1fde40931e:95508:15)
    at BrowserRouter (http://localhost:3000/packages/modules.js?hash=28a15b5a2b97e8a60d0a6cf67d655d1fde40931e:93526:5)

Consider adding an error boundary to your tree to customize error handling behavior.
Visit https://reactjs.org/link/error-boundaries to learn more about error boundaries.

This line was failing because hunt was undefined: https://github.com/deathandmayhem/jolly-roger/blob/main/imports/model-helpers.ts#L14

Adding a !hunt || before the !hunt.submitTemplate worked around the issue, but I'm not sure if there's a higher-level issue with something passing a null hunt to this function that shouldn't be.

zarvox commented 2 months ago

In general, deletion of Hunts is not particularly well-exercised. It exists in the UI, but if I remember correctly it only exists for admins, and we've almost never deleted a hunt. Deleting a hunt doesn't remove it from users.hunts or any other sort of cascading deletion.

In this case the bug is probably more strictly in the non-null assertion at https://github.com/deathandmayhem/jolly-roger/blob/b9bcc2f567691a3eddc63756af9088979c734b7e/imports/client/components/NotificationCenter.tsx#LL964 because hunts.get (and I suppose puzzles.get above it) are not guaranteed to have values for those keys (i.e. precisely in this case, where a Guess object still exists and references a Hunt which is no longer known to the client). There's perhaps other places where the non-null assertion operator (!) can be a lie, either transiently or persistently, in the presence of deletions.

Part of me says that of course we should fix these bugs when we find them, but there's also a part of me that wants to set the expectation that deleting hunts (or other objects that can be referenced by others in general) is probably not something that's going to get exercised a ton, and maybe it'd be better to remove the hunt-deletion feature from the UI instead. Once real humans have started interacting with a hunt in any meaningful way, it's probably a bad idea to delete things out from under them, and it's hard to say what the appropriate user experience you'd expect to generate for any users that have loaded a puzzle from that hunt or sent in guesses for that hunt before it was deleted.

Still, if we have a button for creating a hunt and deleting it, ideally that wouldn't break /everything/.