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 reusing project-codes gets 404/undefined from GQL cache #1084

Conversation

myieye
Copy link
Contributor

@myieye myieye commented Sep 26, 2024

Fixes #1051

It does actually seem to be the case that GQL caching was the problem here (even though I bumped into some weird behaviour in our awaitedQueryStore that I thought was the root of the issue).

I've opened a discussion In the hopes of finding a slightly more elegant solution, but I'm doubtful 🤷. So, for details see that discussion or the many comments in this PR.

There are several bonus fixes in this PR:

  • Text ellipsis truncating wasn't working everywhere, so I fixed and standardized it
  • Project tiles became very ugly with the dev tools open. And they still might on small screens, but meh. That's not as bothersome.
  • The error page was no longer working well for 404 errors. It would start fine, but then immediately update using the 200 from the page load.

Copy link

github-actions bot commented Sep 26, 2024

UI unit Tests

12 tests   12 ✅  0s ⏱️
 4 suites   0 💤
 1 files     0 ❌

Results for commit 493018d.

♻️ This comment has been updated with latest results.

@myieye myieye force-pushed the bug/1051-deleting-a-project-and-creating-a-new-one-with-the-same-code-can-result-in-an-error branch from 0064af5 to 82162c7 Compare September 26, 2024 14:06
@myieye
Copy link
Contributor Author

myieye commented Sep 27, 2024

@hahn-kev You were right, of course 😉. I was totally on the wrong track. I didn't realize that we were excpliticly caching null.

I ended up adding 2 fixes...either one would actually fix this specific error. The reason I included both is:

  1. Pausing the store prevents 2 meaningless and undesireable HTTP requests. So, it just makes sense.
  2. Never caching nulls protects us from any other scenarios that might lead to a null project getting cached (e.g. navigating to a project that doesn't exist or was just deleted), which could lead to the same original error

@hahn-kev
Copy link
Collaborator

I want to get this merged since we have people actively seeing this error. But I also want to have an automated test for this since we've had project deletion broken before. Do you want to do it in this PR or another one?

Copy link
Collaborator

@hahn-kev hahn-kev left a comment

Choose a reason for hiding this comment

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

just one minor request, feel free to merge it then.

frontend/src/lib/gql/gql-client.ts Outdated Show resolved Hide resolved
@myieye myieye force-pushed the bug/1051-deleting-a-project-and-creating-a-new-one-with-the-same-code-can-result-in-an-error branch from a6e6815 to 493018d Compare October 3, 2024 15:04
@myieye myieye merged commit acdc600 into develop Oct 3, 2024
9 checks passed
@myieye myieye deleted the bug/1051-deleting-a-project-and-creating-a-new-one-with-the-same-code-can-result-in-an-error branch October 3, 2024 15:27
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.

deleting a project and creating a new one with the same code can result in an error
2 participants