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.

Sign up for GitHub

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

Jump to bottom

NFT Offers: crash fix #5539

Merged
merged 2 commits into from
Mar 19, 2024
Merged

NFT Offers: crash fix #5539

merged 2 commits into from
Mar 19, 2024

Conversation

benisgold
Copy link
Contributor

@benisgold benisgold commented Mar 19, 2024 •
edited
Loading

Fixes APP-####

What changed (plus any additional context for devs)

  • fixes this crash https://rainbowhaus.slack.com/archives/C0468CDBE75/p1710885267376909
  • the crash was occurring when NaN values were being passed into analytics.identify. segment apparently didn't complain, but rudderstack does NOT like this
  • in the nft offers hook, the mean/median floor difference percentage is calculated for all offers. this involves accessing arrays and dividing those accessed values by other numbers. the problem occurred when there were no NFT offers - trying to access an empty array yields undefined, and dividing undefined by a number yields NaN
  • the fix: added some length checks to the array before trying to access + utilize its values. also renamed some analytics properties bc they were confusing

Screen recordings / screenshots

BEFORE:

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-03-19.at.15.22.44.mp4

AFTER:

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-03-19.at.15.21.28.mp4

What to test

@benisgold benisgold requested review from walmat and dereknelson March 19, 2024 22:30
@benisgold benisgold added release for release blockers and release candidate branches NFT Offers labels Mar 19, 2024
skylarbarrera
skylarbarrera approved these changes Mar 19, 2024
View reviewed changes
Copy link
Contributor

@skylarbarrera skylarbarrera left a comment

Choose a reason for hiding this comment

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

seems fine for now

src/analytics/userProperties.ts Outdated
@@ -23,8 +23,8 @@ export interface UserProperties {
// nft offers
nftOffersAmount?: number;
nftOffersUSDValue?: number;
nftOffersMeanOfferVariance?: number;
nftOffersMedianOfferVariance?: number;
nftOffersMedianFloorDifferencePercentage?: number;
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure if its worth switching these values cause itll mess with the reports we currently have

@skylarbarrera skylarbarrera merged commit 50b5eab into develop Mar 19, 2024
5 of 6 checks passed
@skylarbarrera skylarbarrera deleted the @benisgold/offers-crash-fix branch March 19, 2024 22:44
ibrahimtaveras00 pushed a commit that referenced this pull request Mar 19, 2024
@benisgold @ibrahimtaveras00
NFT Offers: crash fix (#5539)
6e900b8 
* fix

* revert rename
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skylarbarrera skylarbarrera skylarbarrera approved these changes

@dereknelson dereknelson dereknelson approved these changes

@walmat walmat Awaiting requested review from walmat

Assignees
No one assigned
Labels
NFT Offers release for release blockers and release candidate branches
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@benisgold @dereknelson @skylarbarrera