Skip to content

[LWDM] fix(desktop): show price and countervalue for tiny-balance assets#15608

Open
LL782 wants to merge 3 commits intodevelopfrom
fix/LIVE-25763-show-tiny-asset-price
Open

[LWDM] fix(desktop): show price and countervalue for tiny-balance assets#15608
LL782 wants to merge 3 commits intodevelopfrom
fix/LIVE-25763-show-tiny-asset-price

Conversation

@LL782
Copy link
Member

@LL782 LL782 commented Mar 20, 2026

✅ Checklist

  • npx changeset was attached.
  • Covered by automatic tests.
  • Impact of the changes:
    • Price column display in the dashboard asset distribution table

📝 Description

Problem: Assets with very small balances whose % distribution of portfolio rounds to 0 did not show value or price in the asset distribution dashboard. This happened because:

  1. In the desktop Row component, the Price and CounterValue components were conditionally rendered based on distribution being truthy — when distribution was 0, a placeholder dash was shown instead of the actual price/value.

Solution:

  • Removed the conditional rendering in Row.tsx so that Price and CounterValue always render, even when the distribution percentage rounds to 0.

Also done:

  • Add a test to assert that getAssetsDistribution included assets with a counter value of 0. This was not clear before and led to unnecessary changes to Portfolio.ts
Before After
Screenshot 2026-03-20 at 12 07 49 Screenshot 2026-03-20 at 12 07 25

❓ Context


🧐 Checklist for the PR Reviewers

  • The code aligns with the requirements described in the linked JIRA or GitHub issue.
  • The PR description clearly documents the changes made and explains any technical trade-offs or design decisions.
  • There are no undocumented trade-offs, technical debt, or maintainability issues.
  • The PR has been tested thoroughly, and any potential edge cases have been considered and handled.
  • Any new dependencies have been justified and documented.
  • Performance considerations have been taken into account. (changes have been profiled or benchmarked if necessary)

@live-github-bot live-github-bot bot added desktop Has changes in LLD shared-lib Label added for automated tagging of PRs labels Mar 20, 2026
@live-github-bot live-github-bot bot changed the title fix(desktop): show price and countervalue for tiny-balance assets [LWDM] fix(desktop): show price and countervalue for tiny-balance assets Mar 20, 2026
@github-actions
Copy link
Contributor

github-actions bot commented Mar 20, 2026

Web Tools Build Status

Build Status Deployment
Web Tools Build ✅ Deployed https://web-tools-6dy9hgwcx-ledger-hq-prd.vercel.app
Native Storybook Build ⏭️ Skipped
React Storybook Build ⏭️ Skipped

@LL782 LL782 marked this pull request as ready for review March 20, 2026 12:10
@LL782 LL782 requested a review from a team as a code owner March 20, 2026 12:10
Copilot AI review requested due to automatic review settings March 20, 2026 12:10
Copy link
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

Fixes asset distribution display issues for assets whose balances/countervalues round to 0, ensuring they still appear with price and countervalue in the desktop dashboard, and adjusts countervalue handling in the countervalues portfolio logic.

Changes:

  • Update getAssetsDistribution countervalue inclusion check to treat 0 as a valid value.
  • Always render Price and CounterValue in the desktop Asset Distribution row (even when distribution === 0).
  • Add unit tests for the portfolio distribution and the desktop row rendering; add changesets for affected packages.

Reviewed changes

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

Show a summary per file
File Description
libs/live-countervalues/src/portfolio.ts Adjusts countervalue presence check in assets distribution aggregation.
libs/live-countervalues/src/portfolio.test.ts Adds a test intended to cover countervalue === 0 distribution behavior.
apps/ledger-live-desktop/src/renderer/screens/dashboard/AssetDistribution/Row.tsx Removes conditional rendering so Price/CounterValue always render.
apps/ledger-live-desktop/src/renderer/screens/dashboard/AssetDistribution/Row.test.tsx Adds a regression test ensuring Price/CounterValue render when distribution is 0.
.changeset/gentle-coins-appear.md Declares version bump for @ledgerhq/live-countervalues.
.changeset/bright-rows-shine.md Declares version bump for ledger-live-desktop.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 20, 2026

⚠️ E2E tests are required

Changes detected require e2e testing before merge (even before asking for any review).

🖥️ Desktop

-> Run Desktop E2E

  • Select "Run workflow"
  • Branch: fix/LIVE-25763-show-tiny-asset-price
  • Device: nanoSP or stax

📱 Mobile

-> Run Mobile E2E

  • Select "Run workflow"
  • Branch: fix/LIVE-25763-show-tiny-asset-price
  • Device: nanoX

… in distribution

The condition `if (cv)` treated 0 as falsy, excluding assets whose
countervalue rounds to 0 from the distribution list. Use an explicit
null/undefined check so that cv === 0 is kept.
Copilot AI review requested due to automatic review settings March 20, 2026 12:26
@LL782 LL782 force-pushed the fix/LIVE-25763-show-tiny-asset-price branch from a327fd5 to 9b7da6d Compare March 20, 2026 12:26
Copy link
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 6 out of 6 changed files in this pull request and generated 1 comment.

@LL782 LL782 force-pushed the fix/LIVE-25763-show-tiny-asset-price branch from 9b7da6d to 65d1f94 Compare March 20, 2026 13:09
balanceAvailable: accounts.length === 0 || availables.length > 0,
availableAccounts: availables.map(a => a.account),
unavailableCurrencies: [...new Set(unavailableAccounts.map(getAccountCurrency))],
unavailableCurrencies: [...new Set(unavailableAccounts.map(a => getAccountCurrency(a)))],
Copy link
Contributor

Choose a reason for hiding this comment

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

it's the same no?

but more readable

Copy link
Member Author

Choose a reason for hiding this comment

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

I touched this file at one point and SonarQube flagged it.

It does have a good argument:

This can cause unexpected behavior in two scenarios:

  • Function signature changes: If the referenced function is later modified to accept additional parameters, it may start using the index or array arguments unintentionally, leading to different behavior.
  • Unintended parameter usage: The function might already accept multiple parameters for other purposes, causing it to misinterpret the index or array as meaningful input.

Personally I like the shorthand but I don't like getting a big red mark from SonarQube

Since I reverted my changes in this file I could revert this too. WDYT?

import { useCurrencyColor } from "~/renderer/getCurrencyColor";
import styled, { css } from "styled-components";
import CounterValue, { NoCountervaluePlaceholder } from "~/renderer/components/CounterValue";
import CounterValue from "~/renderer/components/CounterValue";
Copy link
Contributor

Choose a reason for hiding this comment

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

InFine it's a product spec that changed right?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure I understand your point

NoCountervaluePlaceholder gets rendered conditionally within Price and CounterValue components

Comment on lines +10 to +13
jest.mock("LLD/hooks/redux", () => ({
useSelector: jest.fn(() => "en-US"),
useDispatch: jest.fn(() => jest.fn()),
}));
Copy link
Contributor

Choose a reason for hiding this comment

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

You can achieve that just with overriding initialState of render()

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. I'll look into this and the similar comments

Comment on lines +19 to +33
jest.mock("~/renderer/hooks/useTheme", () =>
jest.fn(() => ({
colors: {
background: {
card: "#111111",
},
neutral: {
c100: "#ffffff",
},
error: {
c50: "#ff0000",
},
},
})),
);
Copy link
Contributor

Choose a reason for hiding this comment

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

not needed as you use testSetup

Comment on lines +45 to +47
jest.mock("~/renderer/analytics/TrackPage", () => ({
setTrackingSource: jest.fn(),
}));
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

LL782 added 2 commits March 20, 2026 14:27
…ion rows

Remove conditional rendering that hid Price and CounterValue components
when distribution was 0. Tiny-balance assets now display their price
and value instead of a placeholder dash.
Copilot AI review requested due to automatic review settings March 20, 2026 14:28
@LL782 LL782 force-pushed the fix/LIVE-25763-show-tiny-asset-price branch from 65d1f94 to 2b32679 Compare March 20, 2026 14:28
Copy link
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 6 out of 6 changed files in this pull request and generated 3 comments.

Comment on lines +1 to +5
---
"@ledgerhq/live-countervalues": minor
---

Fix asset distribution to include assets with zero countervalue
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

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

This changeset indicates a user-facing fix in @ledgerhq/live-countervalues (“include assets with zero countervalue”), but this PR doesn’t introduce a corresponding runtime behavior change in that package (only a small refactor + tests). Please either remove this changeset or update it to match an actual published behavior/API change, to avoid an unnecessary semver bump and misleading release notes.

Copilot uses AI. Check for mistakes.
Comment on lines +1 to +5
import React from "react";
import { getCryptoCurrencyById } from "@ledgerhq/live-common/currencies/index";
import { render, screen } from "tests/testSetup";
import Row from "./Row";
import type { DistributionItem } from "@ledgerhq/types-live";
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

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

This test uses the generic render helper, which doesn’t provide a seeded countervalues state; that forces the test to mock Price/CounterValue and ends up asserting implementation details. Consider using renderWithMockedCounterValuesProvider (or otherwise seeding countervalues) and asserting on the actual rendered price/countervalue output (or the absence of the “-” placeholder) instead.

Copilot uses AI. Check for mistakes.
Comment on lines +10 to +18
jest.mock("~/renderer/components/Price", () => ({
__esModule: true,
default: (props: unknown) => mockedPrice(props),
}));

jest.mock("~/renderer/components/CounterValue", () => ({
__esModule: true,
default: (props: unknown) => mockedCounterValue(props),
}));
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

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

Mocking ~/renderer/components/Price and ~/renderer/components/CounterValue makes this test tightly coupled to the Row’s internal composition. Prefer exercising the real components with a mocked countervalues provider/state (via existing test helpers) and assert on user-visible output; this will better catch regressions while avoiding component-level mock maintenance.

Copilot uses AI. Check for mistakes.
@sonarqubecloud
Copy link

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

Labels

desktop Has changes in LLD shared-lib Label added for automated tagging of PRs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants