Fix bad claimable challenge state due to nullable max_steps #9461

schottra · 2024-08-15T20:19:09Z

Description

#9432 Made an assumption that since a lot of the code reading max_steps assumed it was a valid number, 0 was a safe default. What was actually happening was that the type was wrong and we were casting around it in apiClient. max_steps should be Nullable, since not all challenge types use it. I updated the type, removed the default value of 0, and fixed all the places in the code that were using the bad assumption that it's always a number.

The symptom exhibited by this bug was that users with no claimable challenges would see a notification dot due to challenges with max_steps=null getting coerced to 0 and a check of current_step_count === max_step_count.

How Has This Been Tested?

Tested locally against staging with accounts that have claimable challenges and no claimable challenges.

changeset-bot · 2024-08-15T20:19:14Z

⚠️ No Changeset found

Latest commit: e05f8b0

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

dharit-tan · 2024-08-15T20:20:56Z

packages/common/src/hooks/useAudioRewards.ts

-  } else if (challenge?.challenge_type === 'aggregate') {
+  } else if (
+    challenge?.challenge_type === 'aggregate' &&
+    challenge?.max_steps !== null


i know this has been discussed but can you remind - why do we need explicit null check instead of !!challenge?.max_steps?

Both are actually fine in this case. I just went for very explicit since the value can explicitly be null or a number.

dharit-tan · 2024-08-15T20:21:33Z

packages/mobile/src/components/challenge-rewards-drawer/ChallengeRewardsDrawerProvider.tsx

          challengeState={challenge.state}
          currentStep={challenge.current_step_count}
-          stepCount={challenge.max_steps}
+          stepCount={challenge.max_steps ?? undefined}


and then can you explain why this one is undefined?

Just because the type in the props is number | undefined and I didn't want to dig into updating the prop shape in case that breaks something else.

dharit-tan · 2024-08-15T20:22:01Z

...c/pages/audio-rewards-page/components/modals/ChallengeRewardsModal/ChallengeRewardsModal.tsx


  const renderProgressBar = () => {
-    if (showProgressBar) {
+    if (showProgressBar && challenge.max_steps) {


if challenge.max_steps is part of showProgressBar do we still need it down here too?

I tried that and TS couldn't pick up that it would not be null here.

audius-infra · 2024-08-15T20:28:35Z

Preview this change https://demo.audius.co/fix-challenge-state

rickyrombo

The null checks make me sad, especially since they aren't actually doing anything... Maybe this is one case where ! coercions make sense? Idk probably fine.

rickyrombo · 2024-08-15T20:24:56Z

packages/common/src/hooks/useAudioRewards.ts

-    } else if (challenge?.challenge_type === 'aggregate') {
+    } else if (
+      challenge?.challenge_type === 'aggregate' &&
+      challenge?.max_steps !== null


fwiw I think the challenge type being aggregate makes max_steps required, so we could do some coercion instead if you think it's cleaner

I think the "true" type for challenges would be a discriminated union of the different types but that's out of scope for this

Yeah, this is my thinking as well. There is a ticket to separate out these types because it's two different cases. This was to get us across the line with the migration without breaking the experience.

…k-endpoints-1 * origin/main: [C-4828] Add timestamp features to comments (#9455) [QA-1504] Fix payout wallet set default (#9467) Add comment replies pagination (#9435) Audius Protocol v0.6.170 change pg string for regex in content (core) (#9471) [C-4929] Add SendIcon component to harmony and harmony native (#9422) Hook up client blast params to sdk (#9466) Don't decrypt plaintext chat.last_message (#9470) fix TS error (#9468) default connection strings (#9465) Fix malformed json in auto-approve script (#9464) Optimistic add chat blasts (#9463) Fix bad claimable challenge state due to nullable max_steps (#9461) Fix comment delete in audius-query (#9462) Fix chat deadlock (#9460) [C-4800] Scaffolding for mobile comments (#9449) Use quotes to escape job name for jq (#9459)

[d712b07] [C-4939] Add comment action menu popup (#9472) JD Francis [641c9c1] [C-4940] Add comment badges (desktop web) (#9475) JD Francis [aca7b5c] [PAY-3315][PAY-3316][PAY-3323] Staking bridge (#9408) Saliou Diallo [bdc139a] [C-4828] Add timestamp features to comments (#9455) JD Francis [3457aeb] [QA-1504] Fix payout wallet set default (#9467) Raymond Jacobson [8c127d0] Add comment replies pagination (#9435) Isaac Solo [c5e250e] [C-4929] Add SendIcon component to harmony and harmony native (#9422) Kyle Shanks [a17b30e] Hook up client blast params to sdk (#9466) Reed [11bf6e3] Don't decrypt plaintext chat.last_message (#9470) Reed [bcaebb0] fix TS error (#9468) JD Francis [bb802b7] Optimistic add chat blasts (#9463) Reed [b5de040] Fix bad claimable challenge state due to nullable max_steps (#9461) Randy Schott [d4c34c3] Fix comment delete in audius-query (#9462) JD Francis [95380d8] [C-4800] Scaffolding for mobile comments (#9449) Sebastian Klingler [fbb4256] [PAY-2917] Migrate user endpoints to SDK, round 3 (#9432) Randy Schott [2c1679a] [PAY-3307] Upgrade chat blasts to chats in sdk (V2) (#9453) Reed [279a9b2] More explicit audience columns for chat blast. (#9454) Steve Perkins [acb857e] Fix bug in profile page 404 (#9456) Raymond Jacobson [c382e82] Don't decrypt plaintext messages (#9448) Reed [172f172] [QA-1495] Fix 404 for users (#9451) Raymond Jacobson [82ab743] [PAY-3227] Unfurl tracks in composer (#9434) Raymond Jacobson [363af71] Compose Solana Client in SDK (#9441) Marcus Pasell [83260f6] Create ChatBlast from web (#9447) Andrew Mendelsohn [19e3bdf] [PAY-3341] Add is_plaintext field for chat message (#9442) Steve Perkins [e25c679] Rename Targeted Message to Chat Blast (#9443) Reed [fc0604c] [C-4964] Comment header changes (#9444) JD Francis [f9e4525] [PAY-3310] Chat blast function in sdk (#9436) Reed [910b655] Move comments context to common (#9440) JD Francis [bb5eaa1] [C-4910] Comments replys & deletes fixes (#9437) JD Francis [1072f5a] [PAY-3106][PAY-2914] Use SDK to withdraw USDC (#9419) Marcus Pasell [1b7bd47] Add social signup feature flag (#9439) Sebastian Klingler [4130f42] Fix duplicate error name in sentry (#9430) Marcus Pasell [54a5a78] [PAY-3300] Chat blast: lazy create thread (#9424) Steve Perkins [ca71d76] [C-4910] Audius-query-ify comments state management (#9423) JD Francis [20a55e9] [PAY-2917][QA-1466] Migrate more users endpoints (#9308) Randy Schott [ab38869] [QA-1434] Fix hanging uploads due to errors on mobile (#9420) Randy Schott [f6ab71c] Add buy button metrics (#9428) Isaac Solo [06ee8a1] Fix pagination args (#9425) Isaac Solo [b91437a] [QA-1501] Fix mobile search input (#9426) Sebastian Klingler [a3c15fd] [C-4932] Add CommentBody component to harmony and harmony-native (#9415) Kyle Shanks

fix bad claimable challenge state due to nullable max_steps

e05f8b0

schottra requested review from dharit-tan and rickyrombo August 15, 2024 20:19

pull-request-size bot added the size/M label Aug 15, 2024

schottra enabled auto-merge (squash) August 15, 2024 20:20

dharit-tan reviewed Aug 15, 2024

View reviewed changes

dharit-tan approved these changes Aug 15, 2024

View reviewed changes

rickyrombo approved these changes Aug 15, 2024

View reviewed changes

schottra merged commit b5de040 into main Aug 15, 2024

schottra deleted the fix-challenge-state branch August 15, 2024 20:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bad claimable challenge state due to nullable max_steps #9461

Fix bad claimable challenge state due to nullable max_steps #9461

Uh oh!

schottra commented Aug 15, 2024

Uh oh!

changeset-bot bot commented Aug 15, 2024

Uh oh!

dharit-tan Aug 15, 2024

Uh oh!

schottra Aug 15, 2024

Uh oh!

dharit-tan Aug 15, 2024

Uh oh!

schottra Aug 15, 2024

Uh oh!

dharit-tan Aug 15, 2024

Uh oh!

schottra Aug 15, 2024

Uh oh!

audius-infra commented Aug 15, 2024

Uh oh!

rickyrombo left a comment

Uh oh!

rickyrombo Aug 15, 2024

Uh oh!

schottra Aug 16, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Fix bad claimable challenge state due to nullable max_steps #9461

Fix bad claimable challenge state due to nullable max_steps #9461

Uh oh!

Conversation

schottra commented Aug 15, 2024

Description

How Has This Been Tested?

Uh oh!

changeset-bot bot commented Aug 15, 2024

⚠️ No Changeset found

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

audius-infra commented Aug 15, 2024

Uh oh!

rickyrombo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants