Skip to content

Discover V2#2671

Open
CassioMG wants to merge 45 commits intomasterfrom
feature/discover-v2
Open

Discover V2#2671
CassioMG wants to merge 45 commits intomasterfrom
feature/discover-v2

Conversation

@CassioMG
Copy link
Copy Markdown
Contributor

@CassioMG CassioMG commented Apr 1, 2026

Summary

Rewrites the Discover feature as a rich, Sheet-based overlay with a Trending carousel, Recent protocols, dApps sections, protocol detail panels, and a first-time welcome modal. This is the extension counterpart to the mobile Discover V2 implementation (stellar/freighter-mobile#780).

Key changes

  • Sheet overlay instead of route: Discover is now a bottom Sheet opened from the Account header (like AssetDetail), replacing the old /discover route
  • Trending carousel: Horizontal scroll with protocol background images and fallback placeholder icons
  • Recent protocols: Tracks the last 5 opened protocols in browser.storage.local, shown on the home screen and in an expanded view with a "Clear recents" option
  • Protocol details panel: SlideupModal showing domain, tags, description, and an "Open" button
  • Welcome modal: Shown once on first visit, persisted via browser.storage.local
  • Data layer: New useDiscoverData hook with useReducer for trending/recent/dApps categorization; new ProtocolEntry type extracted from DiscoverData
  • 8 new components, 2 new hooks, 1 new helper, 13 unit tests

Known issues

  • White border on first Sheet open: A white border shows around Sheet overlays (AssetDetail, Discover) and only goes away after the XLM asset is opened. If you press "shift" on your keyboard the white border comes back. We're still investigating how to properly fix it. This is a pre-existing production bug, not introduced by this PR. See video below as reference.
    UPDATE: this issue has just been fixed here 🎉 with a 1 liner change (after 8 failed attempts) ✅

  • Global scrollbar hiding: The browser's default scrollbar kept appearing over Sheet overlays (AssetDetail, Discover), breaking the visual presentation. Scoping the hiding to individual components did not work — hiding it globally on html, body was the only approach that reliably fixed it. Pending team discussion on whether this trade-off is acceptable. See video below as reference.
    UPDATE: we've decided on keeping the scrollbar hidden for now and further investigate it as part of this task. ✅

Analytics

Metrics match the mobile Discover V2 events (stellar/freighter-mobile#780) for consistent Amplitude dashboards.

Event Properties Triggered when
loaded screen: discover Discover Sheet opens
discover: protocol opened protocolName, url, source, isKnownProtocol User taps "Open" on a protocol row or from a details panel
discover: protocol details viewed protocolName, tags Protocol details panel renders
discover: protocol opened from details protocolName, url User taps "Open" from the details panel
discover: welcome modal viewed Welcome modal renders

Sources: trending_carousel, recent_list, dapps_list, expanded_recent_list, expanded_dapps_list

isKnownProtocol is always true on extension (no search bar for unknown URLs).

Other improvements

  • ViewAppHeader title size changed from size="md" to size="sm" weight="medium" to match updated Figma specs (affects all screens using View.AppHeader)
  • Consistent "X" icon usage and size (Icon.XClose -> Icon.X) throughout the codebase
  • Added explicit ProtocolEntry interface to @shared/api/types
  • Global scrollbar hiding for cleaner UI
  • Portuguese translations for all new strings + 30 previously untranslated strings
  • Removed 2 legacy translation keys

Follow-up work (after initial team review)

  • Analytics/metrics: Add Amplitude events for Discover interactions (open, protocol click)
  • E2E tests: Playwright tests for the full Discover flow (open Sheet, navigate sub-views, open protocol, welcome modal dismissal)

Reference

Videos

Popup experience

pop-up.experience.720p.mov

Fullscreen experience

fullscreen.experience.720p.mov

Scrollbar removal

With scrollbar

with.scrollbar.720p.mov

Without scrollbar

without.scrollbar.720p.mov

### White border bug (known Production issue) fixed here

white.border.bug.720p.mov

CassioMG and others added 26 commits March 30, 2026 20:51
…apping

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…hooks

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…popup mode

The popup closes immediately when a new tab opens, so the storage write
must complete before calling openTab.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…facts

Use height: 100% instead of fixed popup height so the Sheet covers the
full screen in both popup and fullscreen mode. Override Tailwind defaults
that caused a visible white border around the overlay.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@CassioMG CassioMG marked this pull request as ready for review April 1, 2026 01:54
Copilot AI review requested due to automatic review settings April 1, 2026 01:55
@CassioMG CassioMG added the don't review yet wip / tests in progress / missing videos or screenshots / pending self code-review label Apr 1, 2026
@CassioMG CassioMG self-assigned this Apr 1, 2026
@CassioMG CassioMG removed the don't review yet wip / tests in progress / missing videos or screenshots / pending self code-review label Apr 1, 2026
@CassioMG CassioMG requested a review from Copilot April 1, 2026 23:42
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 39 out of 39 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

</div>
<Button
size="lg"
size="md"
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

using the smaller button here (below):

Image

@aristidesstaffieri
Copy link
Copy Markdown
Contributor

Code review

Found 2 issues:

  1. error state from useDiscoverData is returned but never consumed by the Discover component. When the API call fails, isLoading becomes false and all item lists are empty arrays, so the user sees a completely blank Discover panel with no error message, no retry option, and no indication of what went wrong. The error field is explicitly computed via FETCH_ERROR in the reducer (line 52) and returned (line 103), but the destructuring at the call site omits it.

const { isLoading, trendingItems, recentItems, dappsItems, refreshRecent } =
useDiscoverData();

case "FETCH_ERROR":
return { ...state, isLoading: false, error: action.payload };

  1. The METRIC_NAMES.discover constant ("loaded screen: discover") and its routeToEventName mapping were removed, but no replacement metric emission was added when the Discover sheet opens. onDiscoverClick at line 216 only calls setIsDiscoverOpen(true) with no emitMetric. The Discover feature is now completely untracked from an analytics perspective.

isCollectibleHidden={isCollectibleHidden}
onDiscoverClick={() => setIsDiscoverOpen(true)}
/>

viewEditNetwork: "loaded screen: edit network",
viewNetworkSettings: "loaded screen: network settings",
viewAddFunds: "loaded screen: add fund",

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

useState<ProtocolEntry | null>(null);
const [isDetailsOpen, setIsDetailsOpen] = useState(false);

const { isLoading, trendingItems, recentItems, dappsItems, refreshRecent } =
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.

should we use the error state from this hook? Afaict an error from the hook will result in as run time dead screen in this case.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

added here: 010c6fb

error-state.mov

@@ -66,7 +66,6 @@ const routeToEventName = {
METRIC_NAMES.viewAccountMigrationMigrationComplete,
[ROUTES.advancedSettings]: METRIC_NAMES.viewAdvancedSettings,
[ROUTES.addFunds]: METRIC_NAMES.viewAddFunds,
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.

should we still track METRIC_NAMES.discover somewhere even though its not a route? It's probably still useful to have data on discover usage.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

for sure, the PR description mentions that metrics are missing. I've intentionally left the metrics out for now to get an initial review from the team and then implement the same metrics we have on mobile for consistency (which are on the Design doc). So it should soon be done!

Copy link
Copy Markdown
Contributor Author

@CassioMG CassioMG Apr 3, 2026

Choose a reason for hiding this comment

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

added back the existing loaded screen: discover metric + other discover metrics to mimic mobile: 5717f5d

PR description has been updated with an Analytics section

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 just curious - what's the benefit of loading Discover in a Sheet rather than loading it as a separate view?

@CassioMG
Copy link
Copy Markdown
Contributor Author

CassioMG commented Apr 2, 2026

Note: we've decided on keeping the scrollbar hidden for now and further investigate it as part of this task.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 43 out of 43 changed files in this pull request and generated 4 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +64 to +70
const handleOpenProtocol = useCallback(
async (protocol: ProtocolEntry, source: DiscoverSource) => {
trackDiscoverProtocolOpened(protocol.name, protocol.websiteUrl, source);
await addRecentProtocol(protocol.websiteUrl);
await refreshRecent();
openTab(protocol.websiteUrl);
},
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

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

handleOpenProtocol awaits recent-protocol storage updates before calling openTab. If browser.storage.local fails/rejects (e.g., storage quota/permissions issues), this will prevent the user from opening the selected protocol at all. Consider wrapping the recents update/refresh in a try/catch (reporting via Sentry) and always proceeding to openTab so primary navigation isn’t blocked by auxiliary persistence.

Copilot uses AI. Check for mistakes.
Comment on lines +43 to +50
<button
className="ProtocolRow__open-button"
data-testid="protocol-row-open"
onClick={(e) => {
e.stopPropagation();
onOpenClick(protocol);
}}
>
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

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

The "Open" <button> inside ProtocolRow is missing type="button". If this component is ever rendered inside a <form>, the default type="submit" could trigger unintended form submission. Set an explicit type to avoid this class of bugs.

Copilot uses AI. Check for mistakes.
"Current Network": "Current Network",
"Custom": "Custom",
"dApps": "dApps",
"Dapps": "Dapps",
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

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

Both "dApps" and "Dapps" translation keys exist with effectively the same meaning, but there are no code references to t("Dapps") in the repo. Keeping both increases translation/maintenance overhead and can lead to inconsistent UI text. Consider consolidating to a single key (e.g., keep dApps and remove the unused Dapps).

Suggested change
"Dapps": "Dapps",

Copilot uses AI. Check for mistakes.
"Current Network": "Rede Atual",
"Custom": "Personalizado",
"dApps": "dApps",
"Dapps": "Dapps",
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

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

Both "dApps" and "Dapps" translation keys exist with effectively the same meaning, but there are no code references to t("Dapps") in the repo. Keeping both increases translation/maintenance overhead and can lead to inconsistent UI text. Consider consolidating to a single key (e.g., keep dApps and remove the unused Dapps).

Suggested change
"Dapps": "Dapps",

Copilot uses AI. Check for mistakes.
@@ -0,0 +1,28 @@
import browser from "webextension-polyfill";
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.

If you start the extension in dev mode and load the extension in your browser at http://localhost:9000/#/, this code won't be able to run. That's because browser.storage is only available in:

  1. inside the background script

or

  1. a completely bundled extension that you're loading by clicking the little Freighter icon in your toolbar.

Because of this, we keep all interactions with browser.storage in the background script and then transmit them to the UI. For ex: https://github.com/stellar/freighter/blob/master/extension/src/background/messageListener/popupMessageListener.ts#L353

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.

There's also a dual purpose here. By keeping all storage interactions in the background, it makes it easier to track where storage changes were made

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.

It's certainly up for debate if we want to keep using this pattern, but IMO, it's useful to keep this hot reloading dev experience and it's also good practice to keep storage concerns away from the UI

</Text>
<Text as="p" size="sm" weight="regular">
{t(
"These services are operated by independent third parties, not by Freighter or SDF. Inclusion here is not an endorsement. DeFi carries risk, including loss of funds. Use at your own risk.",
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.

NIT: this text is repeated above. could be a variable

onClick={onDismiss}
data-testid="discover-welcome-dismiss"
>
{t("Let's go")}
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.

very NIT: but we generally tried to use smart quotes for apostrophes. This should definitely be enforced by a Claude skill

try {
return new URL(url).hostname;
} catch {
return url;
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.

hm, if the str is not a valid url, this will just return the str. Should we show this if it's not an url?

recentEntries: [],
};

const reducer = (
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.

is it possible to use the request reducer helper for this?

extension/src/helpers/request.ts

Example of how it's implemented: https://github.com/stellar/freighter/blob/master/extension/src/helpers/hooks/useGetHistory.tsx#L15


useEffect(() => {
const checkWelcome = async () => {
const result = await browser.storage.local.get(STORAGE_KEY);
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.

same comment as above in regards to browser.storage

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.

4 participants