Skip to content

refactor(send): unify Tokens/Collectibles loading and error states#899

Open
JakeUrban wants to merge 7 commits into
mainfrom
unify-send-tokens-collectibles-loading-error
Open

refactor(send): unify Tokens/Collectibles loading and error states#899
JakeUrban wants to merge 7 commits into
mainfrom
unify-send-tokens-collectibles-loading-error

Conversation

@JakeUrban

@JakeUrban JakeUrban commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

What

Refactors the Send flow's asset selection screen (TokensCollectiblesInline) so it has a single page-level loading spinner and a single error field, instead of the two independent spinners/errors (one for tokens via BalancesList, one for collectibles) that existed before.

This also removes the "Tokens" and "Collectibles" section headers when the user has none of that asset type — so the empty "No collectibles yet" placeholder under a "Collectibles" header no longer appears when a user has no collectibles.

Behavior

  • One spinner: shows until both tokens and collectibles finish their initial load (token state is lifted from the same useBalancesList hook BalancesList uses internally). Gating on the "no data yet" condition avoids the spinner flashing over already-rendered content on background refreshes.
  • One error field: a token error takes precedence over a collectibles error. Any error replaces the entire view rather than rendering the section that succeeded alongside the error.
  • Section hiding: the Tokens section renders only when the user has token balances; the Collectibles section renders only when the user has collectibles. If both are empty, a single fallback message (transactionTokenScreen.empty) is shown.

Notes / behavior changes

  • An error in either source now blanks the whole view (both sections), per the unified-error requirement. Previously a tokens error only blanked tokens and a collectibles error only blanked collectibles.
  • Gating BalancesList behind "has token balances" means its internal unfunded-account funding notification won't render in this view. This is intentional and safe: the Send action is already disabled for unfunded / zero-balance accounts (HomeScreen Send button is disabled={hasZeroBalance}), so this view is unreachable in that state. The "both empty" fallback is likewise defensive and not reachable in normal use.
  • Added transactionTokenScreen.empty to the en and pt locales.

Testing

  • eslint and tsc --noEmit pass.
  • Unit tests updated to match the refactored component: mocks useBalancesList (token state is now lifted from it), uses the renamed spinner testID (tokens-collectibles-inline-spinner), and replaces the old per-section assertions with coverage of the new unified behavior — single spinner until both sources finish, token-error precedence, hidden Tokens/Collectibles sections when empty, and the combined transactionTokenScreen.empty fallback. All 8 tests pass.

Checklist

PR structure

  • This PR does not mix refactoring changes with feature changes (break it down into smaller PRs if not).
  • This PR has reasonably narrow scope (break it down into smaller PRs if not).
  • This PR includes relevant before and after screenshots/videos highlighting these changes.
  • I took the time to review my own PR.

Testing

  • These changes have been tested and confirmed to work as intended on Android.
  • These changes have been tested and confirmed to work as intended on iOS.
  • These changes have been tested and confirmed to work as intended on small iOS screens.
  • These changes have been tested and confirmed to work as intended on small Android screens.
  • I have tried to break these changes while extensively testing them.
  • This PR adds tests for the new functionality or fixes.

Release

  • This is not a breaking change.
  • This PR updates existing JSDocs when applicable.
  • This PR adds JSDocs to new functionalities.
  • I've checked with the product team if we should add metrics to these changes.
  • I've shared relevant before and after screenshots/videos highlighting these changes with the design team and they've approved the changes.

Replace the two independent loading spinners and error fields in the
Send flow's asset selection (TokensCollectiblesInline) with a single
page-level spinner and a single error field.

- One spinner shows until both tokens and collectibles finish loading.
- One error field: a token error takes precedence over a collectibles
  error, and any error replaces the whole view rather than rendering the
  succeeded section alongside the error.
- The "Tokens" and "Collectibles" section headers are now hidden when the
  user has none of that asset type; a fallback message covers the
  (upstream-unreachable) case where both are empty.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings June 11, 2026 23:04

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

Copy link
Copy Markdown

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: cc297bc4fd

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Copilot AI left a comment

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.

Pull request overview

Refactors the Send flow asset selection UI (TokensCollectiblesInline) to unify token/collectible loading and error handling into a single page-level state, and to hide empty sections so users don’t see headers for asset types they don’t have.

Changes:

  • Lifts token loading/error state via useBalancesList so the screen can show one shared spinner and one shared error message.
  • Conditionally renders the Tokens and Collectibles sections only when there is data; adds a single “both empty” fallback message.
  • Adds transactionTokenScreen.empty translation strings in en and pt.

Reviewed changes

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

File Description
src/components/screens/SendScreen/components/TokensCollectiblesInline.tsx Unifies tokens/collectibles loading + error UI and hides empty sections with a combined empty fallback.
src/i18n/locales/en/translations.json Adds transactionTokenScreen.empty string for the new combined empty fallback.
src/i18n/locales/pt/translations.json Adds transactionTokenScreen.empty string for the new combined empty fallback.

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

@JakeUrban JakeUrban self-assigned this Jun 11, 2026
@github-actions

github-actions Bot commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

iOS Simulator preview build is ready: https://github.com/stellar/freighter-mobile/releases/tag/untagged-47ec19a9e74f785b5823 (SDF collaborators only — install instructions in the release description)

Sync the component tests with the unified loading/error refactor:
mock useBalancesList, rename the spinner testID, and cover the new
single-spinner, token-error-precedence, section-hiding, and combined
empty-state behavior.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@JakeUrban

JakeUrban commented Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the review feedback (thanks @copilot-pull-request-reviewer and @chatgpt-codex-connector). The stale component test was also the failing unit-test job in CI.

__tests__/components/TokensCollectiblesInline.test.tsx is now synced with the refactor in 197af4a:

  • Mocks useBalancesList (the component now lifts token state from it).
  • Uses the renamed spinner testID tokens-collectibles-inline-spinner.
  • Replaces the old per-section assertions with coverage of the new unified behavior: single spinner until both sources finish, token-error precedence, hidden Tokens/Collectibles sections when empty, and the combined transactionTokenScreen.empty fallback.

All 8 tests pass locally; lint and tsc are clean.

@chatgpt-codex-connector

Copy link
Copy Markdown

To use Codex here, create a Codex account and connect to github.

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

Copy link
Copy Markdown

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: 197af4aec0

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Per Codex review: when one source errors while the other is still doing
its initial load, the spinner branch ran before the error branch, masking
the error (and hiding it indefinitely if the second source hangs). Check
the unified errorMessage before showSpinner so any source error replaces
the whole view immediately. Adds a regression test.

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

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

Copy link
Copy Markdown

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: e6aa0f0c91

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/components/screens/SendScreen/components/TokensCollectiblesInline.tsx Outdated
Per Codex review: the collectibles store keeps the previous error set
while a retry is in flight (it only flips isLoading), so the new
error-before-spinner ordering surfaced a stale collectibles error during
retries. Gate the collectibles error on \!collectiblesLoading, mirroring
useBalancesList which already suppresses tokensError while loading. Adds
a regression test.

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

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

Copy link
Copy Markdown

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: f2e6705481

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/components/screens/SendScreen/components/TokensCollectiblesInline.tsx Outdated
JakeUrban and others added 2 commits June 11, 2026 18:04
Per Codex review: selectNetwork doesn't clear `collections`, so during a
post-network-change refetch `visibleCollectibles` can still hold NFTs from
the previous network. Gating the spinner on \!hasCollectibles rendered those
stale, selectable collectibles. Restore the original behavior of showing the
spinner whenever collectibles are loading. Tokens keep their noBalances gate
to match BalancesList and avoid blanking a funded list on refresh.

Also expand test coverage: funded-list-not-blanked-on-refresh, both sections
rendered together, and assert the Tokens header in the collectibles-hidden
case.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

@aristidesstaffieri

Copy link
Copy Markdown
Contributor

@JakeUrban one side effect of this change is that unifying the error states means that a failure to load collectibles results in making tokens also unavailable in the flow. This may be ok but it is a conceptual change from having them fail independently. I think in practice we should not really run into scenarios where collectibles calls fail independently of tokens but imo it's not necessary to block the user from selecting a token in this state, wdyt?

@piyalbasu

piyalbasu commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

@JakeUrban one side effect of this change is that unifying the error states means that a failure to load collectibles results in making tokens also unavailable in the flow. This may be ok but it is a conceptual change from having them fail independently. I think in practice we should not really run into scenarios where collectibles calls fail independently of tokens but imo it's not necessary to block the user from selecting a token in this state, wdyt?

+1 to this comment. Also something to note here is that loading collectibles makes an RPC call but it also loads collectible metadata, which can be hosted on slow servers, etc. This can lead to longer load times where the user can't do anything while they wait for the metadata to resolve. This is the type of UX design we've purposefully moved away from with performance improvements we implemented last year

@JakeUrban

Copy link
Copy Markdown
Contributor Author

Thanks for the feedback @piyalbasu @aristidesstaffieri.

@piyalbasu on your point about blocking on fetching collectibles, I think in this case it makes sense to block on results for both tokens and collectibles because the data is rendered in the same view. Comparatively, we don't block showing tokens while loading collectibles on the home screen because the collectibles aren't being shown immediately. Wdyt?

@aristidesstaffieri thats a fair point. I think we can have a single loading state but have separate error states, so both tokens and collectibles (or their respective errors) appear at the same time, but a failure to fetch one won't block you from selecting the other.

@piyalbasu

Copy link
Copy Markdown
Contributor

Thanks for the feedback @piyalbasu @aristidesstaffieri.

@piyalbasu on your point about blocking on fetching collectibles, I think in this case it makes sense to block on results for both tokens and collectibles because the data is rendered in the same view. Comparatively, we don't block showing tokens while loading collectibles on the home screen because the collectibles aren't being shown immediately. Wdyt?

This is a fair argument. My thought was you can lazy load collectibles at the bottom of the list, which is a fairly common UI practice for views that show long lists with multiple data sources. In this scenario with just tokens and collectibles, we can probably get away with this. I won't block on this suggestion

@CassioMG

Copy link
Copy Markdown
Contributor

I also think we should avoid using a single error component for both to prevent one flow from blocking the other. As for the loading state I think it should probably be fine to use a single spinner as we start fetching both tokens + collectibles at the Home screen so by the time users tap on Send the content should be ready to display most of the time, in case we start often seeing spinners on this Send screen I think we could revisit this.

Make the collectibles spinner gate symmetric with tokens
(collectiblesLoading && \!hasCollectibles) so a background refetch — which
flips isLoading without clearing existing data — no longer blanks the whole
page back to a spinner once collectibles have loaded. The network/account
can't change while the send flow is mounted, so the on-screen data is always
current and the previous stale-cross-network guard is unnecessary.

Also extract renderSectionHeader and renderInlineError helpers to remove the
duplicated section-header and per-section error JSX.

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

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

Copy link
Copy Markdown

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: c1c6e7eb29

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@JakeUrban

Copy link
Copy Markdown
Contributor Author

@CassioMG there is a strange flicker of white in the edges of the iOS simulator screen when transitioning to/from the "Send To" view, see the attached video. I asked Claude to look into it an it came up with this:

Root cause: slide_from_right + iOS modal rounded corners + white native screen background

The Send flow is the only flow that uses a horizontal push animation (slide_from_right) inside an outer stack that's presented as a bottom modal (slide_from_bottom). On iOS, modals presented with slide_from_bottom get rounded top corners (pageSheet). When a screen slides in horizontally, it passes through those rounded corner cutouts — briefly revealing the native screen container's background before React Native's JS thread has painted the dark BaseLayout.

Why it's unique to this flow:

Looking at TransactionTokenScreen.tsx:41:

navigation.push(SEND_PAYMENT_ROUTES.SEND_SEARCH_CONTACTS_SCREEN, {
  transition: ScreenTransition.SlideFromRight,  // ← horizontal, crosses corner areas
});

Compare to every other sub-navigation in the app (ManageTokens, Settings, Swap) — they use getScreenBottomNavigateOptions() which sets animation: "slide_from_bottom". Vertical animations don't cross through the horizontal band where the modal's rounded top corners are, so the artifact never appears.

The technical gap:

SendPaymentNavigator.tsx:53-56 — the navigator's screenOptions don't set a contentStyle:
<SendPaymentStack.Navigator
  screenOptions={{
    header: (props) => <CustomNavigationHeader {...props} />,
    // ← no contentStyle here, so native containers default t
  }}
>

Without contentStyle: { backgroundColor: '#161616' }, the natround is white. React Native's JS components (BaseLayout) paint the dark background asynchronously — by which point the horizontal slide has already crossed the rounded corner zones and the white flash is visible.

Fix:

Add contentStyle to the SendPaymentStack.Navigator's screenOptions in SendPaymentNavigator.tsx:53:

<SendPaymentStack.Navigator
  screenOptions={{
    header: (props) => <CustomNavigationHeader {...props} />,
    contentStyle: { backgroundColor: THEME.colors.background.
  }}
>

You could also add it to the getStackBottomNavigateOptions() roader change affecting all modal stacks. Since only the Sendflow exhibits the issue (due to the slide_from_right crossing modal corners), scoping it to SendPaymentStack.Navigator is the safer targeted fix.

Screen.Recording.2026-06-22.at.9.49.35.AM.mov

@CassioMG

Copy link
Copy Markdown
Contributor

@CassioMG there is a strange flicker of white in the edges of the iOS simulator screen when transitioning to/from the "Send To" view, see the attached video. I asked Claude to look into it an it came up with this:

Root cause: slide_from_right + iOS modal rounded corners + white native screen background

@JakeUrban interesting finding, I see this also happens with the mobile device. I'll take a stab here in a separate branch using your comment as input, hopefully should be a quick-fix

@CassioMG

Copy link
Copy Markdown
Contributor

@CassioMG there is a strange flicker of white in the edges of the iOS simulator screen when transitioning to/from the "Send To" view, see the attached video. I asked Claude to look into it an it came up with this:
Root cause: slide_from_right + iOS modal rounded corners + white native screen background

@JakeUrban interesting finding, I see this also happens with the mobile device. I'll take a stab here in a separate branch using your comment as input, hopefully should be a quick-fix

@JakeUrban FYI, this PR fixes the white flicker: #912

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.

5 participants