(web) enable inserting rows if you own the table #273
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| @@ -1,6 +1,6 @@ | |||
| { | |||
| "name": "@tableland/nonce", | |||
| "version": "1.0.0", | |||
| "version": "1.0.1", | |||
There was a problem hiding this comment.
@asutula In the process of cleaning up this PR I was able to use the recent addition of the nonce debug logging to track down a race condition in how the delta value was being set. I'm 90% sure this is the issue that has been causing all of the test failures. The fix is currently included in this PR, but I can refactor into a separate PR if you'd like?
There was a problem hiding this comment.
No, it's fine. Leave it in here.
| // Need to make sure we increment before returning the nonce value. | ||
| // Otherwise there is a race condition between other processes getting a | ||
| // nonce after the lock release and this call to increment. | ||
| await this.increment(); |
There was a problem hiding this comment.
This was the issue. Since increment was being called after get nonce, the lock release was competing with the increment call and allowing alternate calls to getNonce to get the already used nonce.
| if (transaction.nonce == null) { | ||
| transaction = { ...transaction }; | ||
| transaction.nonce = await this.getNonce("pending"); | ||
| await this.increment(); |
There was a problem hiding this comment.
This was moved inside of the getNonce method, see the comment above for more info.
| @@ -43,11 +43,41 @@ const redis = new Redis({ | |||
| token: process.env.KV_REST_API_TOKEN, | |||
| }); | |||
|
|
|||
| const GLOBAL_TEST_RUNNING = "GLOBAL_TEST_RUNNING"; | |||
|
|
|||
| const ensureSingularTest = async function () { | |||
There was a problem hiding this comment.
I had originally thought that the issue was due to multiple test runners using the same store. This ensures that globally only one nonce test is run at a time. This didn't end up being the actual issue, but it seems like a good thing to have.
| // We can't guarantee that sending 4 transactions from the same wallet will | ||
| // result in a nonce failure. This means that this test might fail | ||
| // occasionally, because of that it's being skipped here. | ||
| test.skip("sending transactions from two processes WITHOUT nonce manager fails", async function () { |
There was a problem hiding this comment.
As per the comment, I think we should skip this test since there's a potential for it to fail even though everything is working correctly. I believe that this test was actually failing because of the above issue with the increment race condition, so if tests start working consistently again we can unskip this in the future.
@joewagner the only error I see is the dev mode error toast provided by Nextjs (which is displayed for uhhandled errors). When I search the changes in this PR, I don't see any code related to triggering our own toast component. Maybe I'm missing somehting? |
There isn't any new code in regard to error handling here. As you mention if there is an error, the error toast shows. |
|
@asutula In the case of some sort of invalid attempt to insert a row, the toast will pop up, then the sql logs show the details of the error in question. I'm open to alternative ways of handling this kind of thing, or fixing if that's not what you are seeing? |
|
Closing for now in favor of a more data driven approach that leverages react-hook-form's features. |
fixes https://linear.app/tableland/issue/ENG-840/enable-editing-one-row-of-data-at-a-time