Automate turn based card draw. #116

ultraviolet10 · 2024-02-01T11:00:23Z

Refers to #107.

Adds in a useEffect in play.tsx that is triggered whenever the turn switches, and triggers the drawCard transaction.
Removes the Draw button since we're now automating the action.

To test: keep ending turns in each tab, the tx is triggered in the other tab immediately (logs confirm this).

@norswap, would be nice to use this to indicate which card has been drawn? (when we have a definitive card list, with IDs and all).

norswap

I see one big problem in testing: the hand doesn't update after the draw. This could be a regression that was introduced earlier? It does update after a hard browser refresh.

I figured out what the issue was, but what do you think if I'd let you try to figure it out? I feel like it would be a good way to get a good grasp on the store logic / data architecture and the way these things update. Let me know what you think and if you want to search, don't hesitate to reach out as soon as you're stuck (or if you just want to use me as a duck to check your reasoning).

norswap · 2024-02-07T12:20:45Z

packages/webapp/src/pages/play.tsx

-    [gameID, playerAddress, setLoading, cancellationHandler])
+      })
+    }
+  }, [cancellationHandler, cantDrawCard, gameData, gameID, playerAddress])


Naively, I would expect this to send the transaction twice in dev mode, because React in dev mode triggers all useEffect twice, and the second invocation to fail on-chain.

Is this not what is observed?

If not, why?

EDIT: Investigated: it seems like the second firing only occurs after the initial one returns, so they don't run in parallel. So I think this might be fined as is.

Not sure if that's a general guarantee when the dependencies change fast though. So for instance say it was possible to receive a gameData update where it's our time to draw and then immediately after another gameData change, there might issues where this fires multiple times in parallel. But it shouldn't fire in parallel.

Could you try this? You can test it by adding a a very very small setTimeout in refreshGameData (or some function called by it) that basically makes a meaningless change to the game data and see if we retrigger the useEffect here.

norswap · 2024-02-08T06:40:06Z

packages/webapp/src/pages/play.tsx

-                <Button variant={"secondary"} className="absolute right-96 bottom-1/2 z-50 !translate-y-1/2 rounded-lg border-[.1rem] border-base-300 font-mono hover:scale-105" disabled={cantDrawCard} onClick={doDrawCard}>
-                  DRAW
-                </Button>
-


It might be nice to give better user feedback, right now the modal will just say "sending transactions" etc. This is ok when the user did an action but here he didn't, so it should probably say "Your turn — drawing card".

I think you're right and using the Sonner is probably a good thing to use here instead of the modal.

Proving is now fast enough that we might want to elide the details ... though I kinda like it. Maybe some kind of visual indicator. For now, probably the easiest thing is "Your turn — drawing card (X)" where X = "proving", "submitting" or "confirming".

I'm not sure we should use the Sonner to show the card drawn, it should appear in the user's hand. Down the line there would be a highlight animation. Like scroll fully to the right of the hand + glow animation. Maybe we can do some of that now? (The scrolling at least would be nice.) And even later, get more schmoozy fancy even (like drop animation).

norswap · 2024-02-15T13:49:39Z

packages/webapp/src/store/derive.ts

@@ -128,9 +127,8 @@ export function getOpponentData(gdata: FetchedGameData|null, player: Address|nul
 /**
 * @see module:store/read#getPrivateInfo
 */
-export function getPrivateInfo(gameID: bigint|null, player: Address|null): PrivateInfo|null {
+export function getPrivateInfo(gameID: bigint|null, player: Address|null, privateInfoStore: PrivateInfoStore): PrivateInfo|null {


You will also need to change getPrivateInfo in read.ts to accommodate this. Let's get into the habit of running make check regularly. Will add an issue for adding github checks.

Yep have this as a change, haven't committed as working on the animation change suggested as well.

Fantastic, keep up the good work :)

norswap · 2024-02-15T13:51:56Z

TODO:

change read.ts to match derive.ts
run make check

After that, let's just merge this, I'll create an issue for the better UX of showing the status of drawing.

norswap

We're getting there :)

But sadly, the scroll animation did not work for me initially, neither Firefox nor Chrome.
(The card was drawn successfully and was displayed in the drawer though.)

Then somehow after a while it started working... No card glow though!

After further testing: there seems to be a dependency on being scrolled enough to the right. When I'm already at the right end of the card drawer, it works, or when I'm close to it. When I'm at the left end it never works.

Also, please remove the scroll back left haha, not sure why do that. Maybe we could scroll back to the previous location though? Feel like staying on the new card would be fine.

packages/webapp/src/components/cards/handCard.tsx

packages/webapp/src/store/derive.ts

packages/webapp/src/store/atoms.ts

packages/webapp/src/store/README.md

packages/webapp/src/hooks/useScrollBox.ts

norswap · 2024-02-18T11:22:42Z

packages/webapp/package.json

You need to update the pnpm lockfile when you update package.json otherwise make setup will complain that the lockfile is not up to date (since it does not specify the version for the new packages).

ultraviolet10 · 2024-02-20T14:36:44Z

@norswap tried adding the timeout to the refreshGameData calls, didn't retrigger the useEffect that performs the drawCard functionality. ✌️

norswap

Alright:

fix package-lock
don't forget to rebase!
kill toast upon draw + pop up card drawer
make check

and we should be good to go!

norswap · 2024-02-21T10:30:23Z

pnpm-lock.yaml

The lock still doesn't seem okay, try running make setup to confirm. Mine still says "Cannot install with "frozen-lockfile" because pnpm-lock.yaml is not up to date with packages/webapp/package.json"

You can run make install to fix, though it's not ideal (as it might updates other dependencies, I think?), but let's do it now.
Normally, just using pnpm install NEW_PKG in the dir of a package (like packages/webapp) should be okay and update the lockfile (which needs to be committed).

norswap · 2024-02-21T10:34:15Z

packages/webapp/src/pages/_app.tsx

There seems to be a regression, probably introduced by the move to shadn where if the global error modal triggers, it does not display.

The browser console gives me

setting error modal: {"title":"Contract execution error","message":"Transaction reverted (drawInitialHand) with InvalidProof(undefined)","buttons":[{"text":"Dismiss"}]}

but the error modal itself is not displayed (or maybe it is but is hidden behind the normal game-creation modal, though a quick look at the DOM makes it seem like this isn't the case).

(As for the error itself, I'm investigating.)

We can leave this for a further issue (but to be prioritized!) and merge this without the fix since it isn't really related to what we're doing here.

norswap · 2024-02-21T10:42:54Z

packages/webapp/src/actions/drawCard.ts

@@ -92,8 +92,6 @@ async function drawCardImpl(args: DrawCardArgs): Promise<boolean> {
  const cards = getCards()!
  console.log(`drew card ${cards[selectedCard]}`)

-  args.setLoading("Generating draw proof ...")
-


I'm going to make the call we need to dismiss it — as if it overlaps with the card being highlighted, which is really "eh".

Also, we should "pop up" the hand (as if it was hovered by the cursor, even if it isn't) at least for the time of the card highlight. I would make the card highlight time a tad longer as well (maybe 3s instead of 2.5?)

…ations

norswap · 2024-02-26T08:21:50Z

Great work, merging this (after rebasing on top of master) 🔥

I think we could improve it further.

e.g. the card appears before the animation in a way that is noticeable, would be great if the animation was more instantaneous with that event, and would also be great if the toast was dismissed immediately when that happens.

I think the main issue there is that the actual update occurs from the refreshGameState whereas animation etc trigger from getting receipts from the transaction, which (at least locally) occurs after. I think this might need to happen as part of a bigger update on how we can signal those things via the first thing that occurs (state update OR getting receipts).

Also testing this I noticed the issue here #128

ultraviolet10 requested a review from norswap February 1, 2024 11:00

norswap requested changes Feb 8, 2024

View reviewed changes

norswap reviewed Feb 15, 2024

View reviewed changes

ultraviolet10 marked this pull request as draft February 15, 2024 13:51

ultraviolet10 requested a review from norswap February 16, 2024 12:55

ultraviolet10 marked this pull request as ready for review February 16, 2024 12:55

norswap requested changes Feb 18, 2024

View reviewed changes

ultraviolet10 requested a review from norswap February 21, 2024 05:42

norswap requested changes Feb 21, 2024

View reviewed changes

ultraviolet10 requested a review from norswap February 22, 2024 16:29

ultraviolet10 added 17 commits February 26, 2024 01:18

feat: automate card draw tx submission on turn change

20da612

fix: check if null

4dc64c0

fix: remove unused import

0f9480a

fix: comment formatting for accurate linking

8d4b4dd

fix: update privateInfo atom dependencies

b40347d

fix: typo

2d8e364

fix: update getPrivateInfo

26a223e

feat: scrolling effect in Hand upon drawing new card, toast notific…

2c8fdef

…ations

fix: minor

58690e3

fix: useScrollbox hook

e13fba6

fix: use accurate type for ref param

7868671

chore: show draw button if useEffect call fails

3a4501b

chore: use placeholder setLoading function

83e9d00

update deps

467a7df

fix: trigger error modal

41c691a

update pnpm-lock

b63a0ec

feat: kill toast upon draw + pop up card drawer

2e4d0fe

norswap force-pushed the ac/auto-card-draw branch from fdf4a9f to 2e4d0fe Compare February 26, 2024 08:18

norswap approved these changes Feb 26, 2024

View reviewed changes

norswap merged commit 8e28481 into master Feb 26, 2024
1 check passed

This was referenced Feb 26, 2024

Fix regression in global error modal #124

Closed

Automate card draw #107

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Automate turn based card draw. #116

Automate turn based card draw. #116

ultraviolet10 commented Feb 1, 2024 •

edited

Loading

norswap left a comment

norswap Feb 7, 2024

norswap Feb 18, 2024

norswap Feb 8, 2024

norswap Feb 15, 2024

ultraviolet10 Feb 15, 2024

norswap Feb 15, 2024

norswap commented Feb 15, 2024

norswap left a comment

norswap Feb 18, 2024

ultraviolet10 commented Feb 20, 2024

norswap left a comment

norswap Feb 21, 2024

norswap Feb 21, 2024

norswap Feb 21, 2024

norswap commented Feb 26, 2024

Automate turn based card draw. #116

Automate turn based card draw. #116

Conversation

ultraviolet10 commented Feb 1, 2024 • edited Loading

norswap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

norswap commented Feb 15, 2024

norswap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ultraviolet10 commented Feb 20, 2024

norswap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

norswap commented Feb 26, 2024

ultraviolet10 commented Feb 1, 2024 •

edited

Loading