Skip to content

[LWD] feat(desktop): hide operations list in portfolio when ff on#15612

Open
mcayuelas-ledger wants to merge 1 commit intodevelopfrom
feat/wallet40-hide-operation-list
Open

[LWD] feat(desktop): hide operations list in portfolio when ff on#15612
mcayuelas-ledger wants to merge 1 commit intodevelopfrom
feat/wallet40-hide-operation-list

Conversation

@mcayuelas-ledger
Copy link
Contributor

✅ Checklist

  • npx changeset was attached.
  • Covered by automatic tests.
  • Impact of the changes:
    • ...

📝 Description

Hide Operations List in Portfolio when FF in ON

❓ 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)

@mcayuelas-ledger mcayuelas-ledger requested a review from a team as a code owner March 20, 2026 13:08
Copilot AI review requested due to automatic review settings March 20, 2026 13:08
@live-github-bot live-github-bot bot added the desktop Has changes in LLD label Mar 20, 2026
@live-github-bot live-github-bot bot changed the title feat(desktop): hide operations list in portfolio when ff on [LWD] feat(desktop): hide operations list in portfolio when ff on Mar 20, 2026
@mcayuelas-ledger mcayuelas-ledger force-pushed the feat/wallet40-hide-operation-list branch from 9704d70 to 58c6ab4 Compare March 20, 2026 13:09
@github-actions
Copy link
Contributor

github-actions bot commented Mar 20, 2026

Web Tools Build Status

Build Status Deployment
Web Tools Build ⏭️ Skipped
Native Storybook Build ⏭️ Skipped
React Storybook Build ⏭️ Skipped

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

Adds a Wallet 4.0 feature-flag-driven switch in the Desktop MVVM Portfolio to hide the legacy Operations list when the corresponding FF param is enabled (LIVE-26176).

Changes:

  • Exposes shouldDisplayOperationsList from useWalletFeaturesConfig("desktop") through the Portfolio ViewModel.
  • Updates PortfolioView to conditionally render OperationsList based on that flag, and adjusts container padding.
  • Adds a changeset for the release.

Reviewed changes

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

File Description
apps/ledger-live-desktop/src/mvvm/features/Portfolio/hooks/usePortfolioViewModel.ts Surfaces the wallet feature config boolean into the Portfolio MVVM ViewModel result.
apps/ledger-live-desktop/src/mvvm/features/Portfolio/PortfolioView.tsx Uses the new flag to hide/show the Operations list (and tweaks layout padding).
.changeset/thick-bananas-talk.md Declares package version bumps for the release.

Comment on lines 36 to 38
const shouldDisplayAddAccountCta = totalAccounts === 0 && isWallet40Enabled;
const shouldDisplayOperationsListCondition = !shouldDisplayOperationsList && totalOperations > 0;

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.

shouldDisplayOperationsList is named like a positive flag, but the render condition is inverted (!shouldDisplayOperationsList). This makes the code easy to misread and increases the chance of future regressions. Consider renaming the local boolean to something like shouldRenderLegacyOperationsList (or invert earlier in the ViewModel and expose a prop with the correct semantics).

Copilot uses AI. Check for mistakes.
Copy link
Member

Choose a reason for hiding this comment

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

I was going to say something similar. This reads really strangely currently.

If it makes sense, shouldRenderLegacyOperationsList is a good suggestion, IMO

shouldDisplayGraphRework,
shouldDisplayQuickActionCtas,
shouldDisplayAssetSection,
shouldDisplayOperationsList,
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.

shouldDisplayOperationsList is destructured without a default. In non-ViewModel usages (notably tests that spread defaultProps), this can be undefined, which currently behaves as false and unintentionally renders the operations list. Consider providing a default (shouldDisplayOperationsList = false) to keep behavior deterministic when the prop is omitted.

Suggested change
shouldDisplayOperationsList,
shouldDisplayOperationsList = false,

Copilot uses AI. Check for mistakes.

{totalOperations > 0 && (
{shouldDisplayOperationsListCondition && (
<OperationsList
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.

The PR introduces new behavior (hiding the OperationsList based on a wallet feature flag), but there’s no corresponding automated coverage in the existing Portfolio integration test suite. Please add an assertion that OperationsList is not rendered when the flag is enabled, and is rendered when disabled, to prevent regressions.

Suggested change
<OperationsList
<OperationsList
data-testid="portfolio-operations-list"

Copilot uses AI. Check for mistakes.
@@ -0,0 +1,6 @@
---
"ledger-live-desktop": minor
"@ledgerhq/live-common": minor
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 bumps @ledgerhq/live-common, but this PR doesn’t appear to modify any files under libs/ledger-live-common. If live-common isn’t actually changed, remove it from the changeset to avoid an unnecessary release/version bump.

Suggested change
"@ledgerhq/live-common": minor

Copilot uses AI. Check for mistakes.
@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: feat/wallet40-hide-operation-list
  • Device: nanoSP or stax

@mcayuelas-ledger mcayuelas-ledger force-pushed the feat/wallet40-hide-operation-list branch from 58c6ab4 to 0e60848 Compare March 20, 2026 13:14
Copy link
Member

@LL782 LL782 left a comment

Choose a reason for hiding this comment

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

Happy to approve as I don't want to slow things down but PortfolioView.tsx:29 reads really strangely atm

Comment on lines 36 to 38
const shouldDisplayAddAccountCta = totalAccounts === 0 && isWallet40Enabled;
const shouldDisplayOperationsListCondition = !shouldDisplayOperationsList && totalOperations > 0;

Copy link
Member

Choose a reason for hiding this comment

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

I was going to say something similar. This reads really strangely currently.

If it makes sense, shouldRenderLegacyOperationsList is a good suggestion, IMO

Copilot AI review requested due to automatic review settings March 20, 2026 13:18
@mcayuelas-ledger mcayuelas-ledger force-pushed the feat/wallet40-hide-operation-list branch from 0e60848 to 709b5a7 Compare March 20, 2026 13:18
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 3 out of 3 changed files in this pull request and generated 1 comment.

Comment on lines 26 to 35
shouldDisplayGraphRework,
shouldDisplayQuickActionCtas,
shouldDisplayAssetSection,
shouldDisplayOperationsList,
isWallet40Enabled,
accounts,
filterOperations,
t,
isClearCacheBannerVisible,
}: PortfolioViewModelResult) {
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.

PortfolioView now requires shouldDisplayOperationsList via PortfolioViewModelResult. This breaks the existing direct PortfolioView renders in __integrations__/Portfolio.integration.test.tsx (defaultProps doesn’t include this field) and will cause TypeScript errors in CI. Either update those test props to include shouldDisplayOperationsList, or decouple PortfolioView’s prop type from the ViewModel result (e.g., define a dedicated PortfolioViewProps with a default for this flag).

Copilot uses AI. Check for mistakes.
LL782
LL782 previously approved these changes Mar 20, 2026
@mcayuelas-ledger mcayuelas-ledger force-pushed the feat/wallet40-hide-operation-list branch from 709b5a7 to 90aa124 Compare March 20, 2026 14:46
Base automatically changed from feat/wallet40-operation-list-ff to develop March 20, 2026 15:50
@mcayuelas-ledger mcayuelas-ledger dismissed LL782’s stale review March 20, 2026 15:50

The base branch was changed.

@mcayuelas-ledger mcayuelas-ledger force-pushed the feat/wallet40-hide-operation-list branch from 90aa124 to afe72a7 Compare March 20, 2026 15:53
@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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants