feat(discover): Discover v2 card and section analytics#7557
feat(discover): Discover v2 card and section analytics#7557DanielSinclair wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 74422c8. Configure here.
| outcome: order.outcome, | ||
| }); | ||
| } | ||
| : noopOrderPress; |
There was a problem hiding this comment.
Analytics handler construction duplicated across three layout files
Medium Severity
The onCardPress and onOrderPress handler construction — including the discoverCardPressed tracking, trackPlacementInteraction call, discoverPredictionOrderPressed tracking, and the noopCardPress/noopOrderPress fallbacks — is copy-pasted nearly identically across MarketCarousel, MarketGrid, and MarketList. A future change to the tracking payload (e.g. adding a field to discoverCardPressed) would need to be applied in three places, risking inconsistency. This ~40-line block could be extracted into a shared factory function.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 74422c8. Configure here.
e1c8e71 to
c88a9f0
Compare
74422c8 to
53e9246
Compare
i1skn
left a comment
There was a problem hiding this comment.
left some comments, mainly to remove code duplication
|
|
||
| import { ShowMoreButton, ShowMoreCellEnterAnimation } from './ShowMoreButton'; | ||
|
|
||
| function noopCardPress(): undefined { |
There was a problem hiding this comment.
nit: I think we can just have single noopPress here
| const listItem = <View>{renderItem(item)}</View>; | ||
| const onCardPress: CardPressHandler = placement | ||
| ? metadata => { | ||
| analytics.track(event.discoverCardPressed, { |
There was a problem hiding this comment.
I see we repeat it here, I think we can move it to something like trackCardPress to encapsulate these two large calls there inside
There was a problem hiding this comment.
This is also going to break any downstream component memoization, creating these functions inline
53e9246 to
6c25cf5
Compare
c88a9f0 to
455bcd4
Compare
i1skn
left a comment
There was a problem hiding this comment.
approving this, as none of my comments are critical, but please take a look on them
|
|
||
| const HORIZONTAL_PADDING = 12; | ||
| const CARD_GAP = HORIZONTAL_PADDING; | ||
| const SCROLL_DEBOUNCE_MS = time.ms(500); |
There was a problem hiding this comment.
This seem too frequent. This is effectively tracking every scroll interaction.
6c25cf5 to
0b43bb6
Compare
455bcd4 to
27a223e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7fe26ebb0a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| marketId: row.market.id, | ||
| marketName: row.market.question, | ||
| marketSlug: row.market.slug, | ||
| outcome: row.title, |
There was a problem hiding this comment.
Report the selected market outcome
For multi-market prediction widgets, row.title is the display label derived from groupItemTitle/question in getOutcomeRows, not the actual selected outcome at row.outcomeIndex. In those cases the order analytics records labels like the threshold/team row instead of the outcome being bought, while the navigation uses outcomeIndex to open the order sheet for row.market.outcomes[row.outcomeIndex]; this corrupts discover.prediction_order_pressed outcome data for those widgets.
Useful? React with 👍 / 👎.
7fe26eb to
702b335
Compare
27a223e to
5a2337d
Compare
702b335 to
def7421
Compare
1d29e79 to
c5e9d50
Compare
def7421 to
36a2091
Compare
The card and order press handlers each fell back to a separate no-op (noopCardPress / noopOrderPress) when a section had no placement. Both are zero-arg `(): undefined` functions, so a single shared `noopPress` satisfies the CardPressHandler and OrderPressHandler fallbacks in all three layouts (carousel, grid, list), per i1skn's review.
c5e9d50 to
ef8527e
Compare
36a2091 to
1a1bc50
Compare



What changed
CardPressHandler(predictions via anOrderPressHandler) passed into eachrenderItem.onPressSeeAll+surfacetoonPress+placement+section+surfaceId.PlacementItemAnalyticsMetadatatype; tightenssurfaceIdprops toSurfaceId.