Skip to content

feat(discover-v2): scaffold featured markets carousel placements#7417

Open
DanielSinclair wants to merge 4 commits intodaniel/placements-apifrom
daniel/discover-v2-market-carousels
Open

feat(discover-v2): scaffold featured markets carousel placements#7417
DanielSinclair wants to merge 4 commits intodaniel/placements-apifrom
daniel/discover-v2-market-carousels

Conversation

@DanielSinclair
Copy link
Copy Markdown
Contributor

@DanielSinclair DanielSinclair commented Apr 30, 2026

APP-3670

What changed

  • Removes the old Perps/Predictions feature cards from the Trending section
  • Adds MarketCarousel — a generic horizontal FlatList with shimmer skeleton loading, a "See all" action, and variable-width item support
  • DiscoverHome subscribes to useDiscoverPlacementAvailabilityStore and renders a MarketCarousel section for each active placement type; shows MarketCarousel skeleton slots while placement data is loading
  • Actual card components land in Android renaming #5 (Perps) and Add spinner for fetching transactions  #7 (Predictions) — link once PRs are open

Screen recordings / screenshots

What to test

  • Old Perps/Predictions feature cards are gone from Trending
  • With placements enabled: shimmer placeholder rows visible for Perps and Predictions
  • With placements disabled or on testnet: sections hidden; existing Trending Tokens, mints, learn cards unaffected

Copy link
Copy Markdown
Contributor Author

DanielSinclair commented Apr 30, 2026

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 91c9a66f8d

ℹ️ 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".

Comment on lines +48 to +50
const showPerpsPlacement = availability.perps && isLoading;
const showPredictionsPlacement = availability.predictions && isLoading;
const showPlacements = showPerpsPlacement || showPredictionsPlacement;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Keep placement sections rendered after fetch completes

showPerpsPlacement and showPredictionsPlacement are gated on isLoading, so both sections are unmounted as soon as usePlacementsStore reaches success (or when cached state hydrates as non-loading). In practice this means enabled placements are only visible during a short loading window and can disappear permanently for warm-cache users, which defeats the carousel placement surface this change is introducing.

Useful? React with 👍 / 👎.

itemHeight={CARD_HEIGHT}
itemWidth={CARD_WIDTH}
renderItem={(_item, _helpers) => <Box />}
onSeeAll={() => undefined}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Remove dead “See all” CTA until navigation exists

Both placement carousels render a visible “See all” action but pass onSeeAll={() => undefined}, so taps do nothing besides analytics. For users with placements enabled, this exposes a non-functional primary affordance and creates a dead-end interaction; the button should be hidden/disabled until a destination is wired.

Useful? React with 👍 / 👎.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 30, 2026

🧪 Flashlight Performance Report (AWS Device Farm)

🔀 Commit: 3133b38

📎 View Artifacts

Metric Current Δ vs Baseline
Time to Interactive (TTI) 5599 ms
Average FPS 56.52
Average RAM 395.5 MB

@github-actions
Copy link
Copy Markdown

Launch in simulator or device for bdb8dec

Clears out the old Perps/Predictions feature card surfaces before the new placement carousels are introduced.
Pure path migration — isolated so reviewers see only the move.
Generic horizontal carousel component with shimmer skeleton loading, See All button, analytics trackPress slot, and variable-width item support. Used by both Perps and Predictions placements.
Adds Perps and Predictions carousel sections to DiscoverHome driven by the availability store. Renders skeleton placeholders while placement data loads; sections are hidden once loaded since composition stores land in the domain branches.
@DanielSinclair DanielSinclair force-pushed the daniel/discover-v2-market-carousels branch from b5f5ba8 to 6bbfc95 Compare April 30, 2026 11:42
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6bbfc95b47

ℹ️ 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".


const testNetwork = useSyncDiscoverPlacementAvailabilityNetwork();
const availability = useDiscoverPlacementAvailability();
const isLoading = usePlacementsStore(state => state.status === 'loading' || state.status === 'idle');
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Gate placements store subscription behind availability

usePlacementsStore is subscribed unconditionally, so createQueryStore will still kick off a placements fetch even when placements can never be shown (for example on testnet, or when discover_placements_enabled is off). This introduces unnecessary Firestore traffic and background work on every Discover visit in those environments; the subscription/fetch should be conditioned on placement availability instead of always mounting.

Useful? React with 👍 / 👎.

@github-actions
Copy link
Copy Markdown

Launch in simulator or device for 3133b38

@DanielSinclair DanielSinclair requested review from maxbbb and removed request for akc2267 April 30, 2026 23:23
Copy link
Copy Markdown
Contributor

@ibrahimtaveras00 ibrahimtaveras00 left a comment

Choose a reason for hiding this comment

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

Looks good on both OS's ✅

Comment on lines +73 to +83
analytics.track(event.discoverPlacementCardPressed, {
placementId,
placementScreen: placement?.screen,
placementTitle: title,
itemOrder: item.order,
marketId,
marketName,
marketSlug,
marketSymbol,
marketType: item.ref.source,
});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not sure much is gained here vs. just passing in the whole placement object as a "placement" field. If you want to query this data in amplitude you're going to want to separate out the top level type (perps vs. polymarket) anyway, so attempting to normalize the data here and committing to do that for all future iterations seems not necessary. Open to hearing if there is an argument for this shape though.

Would also allow us to have just one event "placementCardPressed" that would work on other future screens (since placements have a "screen" field).

return <View style={{ width: itemWidths[index] ?? itemWidth }}>{renderItem(item, { trackPress })}</View>;
},
[itemWidth, itemWidths, placement, placementId, renderItem, title]
);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This whole trackPress pattern is not ideal imo. Would be much cleaner to let the individual components own the onPress or to just wrap the view here in a <ButtonPressAnimation> and adding a pressProps prop if you want to let consumers customize the scaleTo or anything else (which I don't think we even need).

))}
</View>
) : (
<Animated.FlatList
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We're not using animated animated props, should just be a normal FlatList

Comment on lines +178 to +180
contentContainer: {
paddingHorizontal: CAROUSEL_HORIZONTAL_PADDING,
},
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You can just put gap: CARD_GAP here and remove all the ItemSeparatorComponent related logic.

getItemWidth?: (item: T) => number;
keyExtractor: (item: T) => string;
loading?: boolean;
onSeeAll: () => void;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would make the prop name clear that it's a press event (ie onPressSeeAll)

itemHeight?: number;
itemWidth?: number;
getItemWidth?: (item: T) => number;
keyExtractor: (item: T) => string;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For any prop types that are meant to be directly drilled to the list, I would prefer using the list props directly through = Pick<FlatListProps<T>, 'data' | 'keyExtractor'> & { other props }


export const HORIZONTAL_PADDING = 20;

const keyExtractor = (item: PlacementItem) => `${item.ref.source}:${item.ref.id}`;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

prefer function keyExtractor declaration for module level definitions.

Comment on lines +58 to +70
{showPerpsPlacement && (
<MarketCarousel
title={i18n.t(i18n.l.discover.placements.perps_title)}
placementId={PLACEMENT_IDS.PERPS}
data={[] as PlacementItem[]}
keyExtractor={keyExtractor}
itemHeight={CARD_HEIGHT}
itemWidth={CARD_WIDTH}
renderItem={(_item, _helpers) => <Box />}
onSeeAll={() => undefined}
loading={isLoading}
/>
)}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I know not the general pattern in this file (it's pretty old) but it would better to put the specific placement logic into its own components, so in this case PerpsMarketCarousel that subscribes to the state needed to decided if it should render, and just return null if not. Same thing for the predictions carousel.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants