✨ (llm|lld): delete local app data on uninstall and uninstallAll #7744

valpinkman · 2024-09-04T09:50:38Z

✅ Checklist

npx changeset was attached.
Covered by automatic tests.
Impact of the changes:
- delete app data when restore is finished (wip: handle UserRefusedOnDevice case)
- updated the apprunner state to know when to delete app data backup during uninstall
- refactor execWithTransport in its own file
- update lld to use new flow
- update llm to use new flow

📝 Description

This PR makes sure to remove local app data when uninstalling app through the manager on LLD/LLM + all changes required in LLC to make it work

❓ Context

JIRA or GitHub link: LIVE-13454 [LIVE-12688]

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

vercel · 2024-09-04T09:50:41Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
web-tools	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Sep 9, 2024 1:02pm

4 Skipped Deployments

Name	Status	Preview	Updated (UTC)
ledger-live-docs	⬜️ Ignored (Inspect)	Visit Preview	Sep 9, 2024 1:02pm
ledger-live-github-bot	⬜️ Ignored (Inspect)	Visit Preview	Sep 9, 2024 1:02pm
native-ui-storybook	⬜️ Ignored (Inspect)	Visit Preview	Sep 9, 2024 1:02pm
react-ui-storybook	⬜️ Ignored (Inspect)	Visit Preview	Sep 9, 2024 1:02pm

jiyuzhuang · 2024-09-05T14:00:40Z

libs/ledger-live-common/src/device/use-cases/appDataBackup/restoreAppData.ts

-          subscriber.complete();
+        }),
+        // Delete the app data from the storage
+        switchMap(() => deleteAppData(appName, deviceModelId, storageProvider)),


Interaction with data storage in this file is counter to the initial pattern.
This restoreAppData as well as backupAppData are dedicated to data stream programming. I think that the data deletion use case should be enchained in restoreAppDataUseCase where all the parameters are already present.

In addition, the deleting the local data is not part of the data restoration workflow. It is an operation independent and should not be injected into normal restoration workflow.

make sense, i will fix this

ofreyssinet-ledger · 2024-09-09T09:47:09Z

libs/ledger-live-common/src/hw/uninstallAppWithBackup.ts

@@ -12,9 +12,9 @@ export default function uninstallAppWithBackup(
  app: ApplicationVersion | App,
  deviceId: DeviceModelId,
  storage: StorageProvider<AppStorageType>,
-  shouldBackup: boolean = true,
+  deleteAppDataBackup: boolean = false,


[SHOULD] The naming of deleteAppDataBackup is a bit strange because it doesn't match with what setting it to true will do, it doesn't actually delete any app data here (shouldBackup seemed much better to me it that's how it's supposed to work)

ofreyssinet-ledger · 2024-09-09T09:55:46Z

libs/ledger-live-common/src/device/use-cases/appDataBackup/restoreAppData.test.ts

+        expect(events).toHaveLength(5);
+        expect(events[0]).toEqual({
+          type: RestoreAppDataEventType.AppDataInitialized,
+        });
+        expect(events[1]).toEqual({
+          type: RestoreAppDataEventType.Progress,
+          data: expect.any(Number),
+        });
+        expect(events[2]).toEqual({
+          type: RestoreAppDataEventType.Progress,
+          data: expect.any(Number),
+        });
+        expect(events[3]).toEqual({
+          type: RestoreAppDataEventType.Progress,
+          data: expect.any(Number),
+        });
+        expect(events[4]).toEqual({
+          type: RestoreAppDataEventType.AppDataRestored,
+        });
+        done();
+      },


[SHOULD] if expect throws, done will never be called and the test will just be timing out, without an easy to read error message. In async tests with done, all expect should be surrounded with a try/catch with something like catch(caughtError) { done(caughtError) }

[ASK] Unrelated to the first comment: could use one expect(events).toEqual(expectedArray) instead of 5 expects, no ? Not crucial though

ofreyssinet-ledger · 2024-09-09T09:57:27Z

libs/ledger-live-common/src/device/use-cases/appDataBackup/restoreAppData.test.ts

+        });
+        expect(events[4]).toEqual({
+          type: RestoreAppDataEventType.AppDataRestored,
+        });


same comments as above

libs/ledger-live-common/src/device/use-cases/appDataBackup/restoreAppDataUseCase.test.ts

ofreyssinet-ledger · 2024-09-09T09:59:37Z

libs/ledger-live-common/src/apps/types.ts

@@ -10,6 +10,7 @@ export type ExecArgs = {
  app: App;
  modelId?: DeviceModelId;
  storage?: StorageProvider<AppStorageType>;
+  deleteAppDataBackup?: boolean;


[COULD] maybe add some TSdoc on those args

ofreyssinet-ledger · 2024-09-09T10:03:19Z

libs/ledger-live-common/src/device/use-cases/execWithTransport.ts

+import uninstallApp from "../../hw/uninstallApp";
+import uninstallAppWithBackup from "../../hw/uninstallAppWithBackup";
+
+export const execWithTransport =


[COULD] Maybe it's worth adding a bit of TSDoc to explain what this does, and even potentially rename it to something less mysterious...

ofreyssinet-ledger

Some things to fix in async unit tests.
Also I think the name of the deleteAppDataBackup is maybe a bit too vague.
And I'm not sure I understand where the actual deletion of locally stored app data is done for app uninstallation, deleteAppDataUseCaseDI is never called in uninstall flows as far as I understand 🤔

valpinkman · 2024-09-09T12:10:45Z

Some things to fix in async unit tests. Also I think the name of the deleteAppDataBackup is maybe a bit too vague. And I'm not sure I understand where the actual deletion of locally stored app data is done for app uninstallation, deleteAppDataUseCaseDI is never called in uninstall flows as far as I understand 🤔

Things are done here https://github.com/LedgerHQ/ledger-live/pull/7744/files#diff-cadd4d39653d7214faf8f5dd226d1a6e9c4fc35c45e66a08787170bcc9097d71 to know how to handle each case. (the state)
And there is no "deletion" when uninstalling https://github.com/LedgerHQ/ledger-live/pull/7744/files#diff-64aaf9798ce046311b2798bcaa0ebd94cf9c3308faf867ac000a861a6c150a87R19 we just skip the backup.

But fair point I will rename deleteAppDataBackup to make more sense

ofreyssinet-ledger

nice 👍

valpinkman requested review from a team as code owners September 4, 2024 09:50

github-actions bot assigned valpinkman Sep 4, 2024

live-github-bot bot added desktop Has changes in LLD mobile Has changes in LLM common Has changes in live-common labels Sep 4, 2024

valpinkman changed the title ~~✨ (lld): delete local app data on uninstall and uninstallAll~~ ✨ (desktop): delete local app data on uninstall and uninstallAll Sep 4, 2024

vercel bot deployed to Preview – web-tools September 4, 2024 09:55 View deployment

valpinkman force-pushed the feat/delete-app-data-after-restore branch from 9d11323 to 938e6b6 Compare September 4, 2024 12:10

live-github-bot bot added the cli label Sep 4, 2024

vercel bot had a problem deploying to Preview – web-tools September 4, 2024 12:11 Failure

valpinkman force-pushed the feat/delete-app-data-after-restore branch from 938e6b6 to 8ba07fc Compare September 4, 2024 12:50

vercel bot deployed to Preview – web-tools September 4, 2024 12:54 View deployment

valpinkman changed the title ~~✨ (desktop): delete local app data on uninstall and uninstallAll~~ ✨ (llm|lld): delete local app data on uninstall and uninstallAll Sep 4, 2024

valpinkman force-pushed the feat/delete-app-data-after-restore branch 2 times, most recently from d2884b6 to 61a89c9 Compare September 5, 2024 12:55

jiyuzhuang reviewed Sep 5, 2024

View reviewed changes

valpinkman force-pushed the feat/delete-app-data-after-restore branch from 61a89c9 to c9b5761 Compare September 5, 2024 15:04

vercel bot deployed to Preview – web-tools September 5, 2024 15:08 View deployment

valpinkman force-pushed the feat/delete-app-data-after-restore branch from c9b5761 to 2d7d5de Compare September 5, 2024 15:21

vercel bot deployed to Preview – web-tools September 5, 2024 15:23 View deployment

✨ (lld): delete local app data on uninstall and uninstallAll

fcd8d52

valpinkman force-pushed the feat/delete-app-data-after-restore branch from 2d7d5de to 6bd15b3 Compare September 9, 2024 09:43

vercel bot deployed to Preview – web-tools September 9, 2024 09:51 View deployment

ofreyssinet-ledger requested changes Sep 9, 2024

View reviewed changes

✨ (llm): delete local app data on uninstall and uninstallAll

7e81505

valpinkman force-pushed the feat/delete-app-data-after-restore branch from 6bd15b3 to 7e81505 Compare September 9, 2024 12:58

vercel bot deployed to Preview – web-tools September 9, 2024 13:02 View deployment

ofreyssinet-ledger approved these changes Sep 9, 2024

View reviewed changes

valpinkman merged commit 1e0eece into develop Sep 9, 2024
58 of 59 checks passed

valpinkman deleted the feat/delete-app-data-after-restore branch September 9, 2024 13:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ (llm|lld): delete local app data on uninstall and uninstallAll #7744

✨ (llm|lld): delete local app data on uninstall and uninstallAll #7744

valpinkman commented Sep 4, 2024 •

edited

Loading

vercel bot commented Sep 4, 2024 •

edited

Loading

jiyuzhuang Sep 5, 2024

jiyuzhuang Sep 5, 2024

valpinkman Sep 5, 2024

ofreyssinet-ledger Sep 9, 2024 •

edited

Loading

valpinkman Sep 9, 2024

ofreyssinet-ledger Sep 9, 2024

ofreyssinet-ledger Sep 9, 2024

valpinkman Sep 9, 2024

ofreyssinet-ledger Sep 9, 2024

valpinkman Sep 9, 2024

ofreyssinet-ledger Sep 9, 2024

ofreyssinet-ledger Sep 9, 2024

ofreyssinet-ledger left a comment •

edited

Loading

valpinkman commented Sep 9, 2024

ofreyssinet-ledger left a comment

✨ (llm|lld): delete local app data on uninstall and uninstallAll #7744

✨ (llm|lld): delete local app data on uninstall and uninstallAll #7744

Conversation

valpinkman commented Sep 4, 2024 • edited Loading

✅ Checklist

📝 Description

❓ Context

🧐 Checklist for the PR Reviewers

vercel bot commented Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ofreyssinet-ledger Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ofreyssinet-ledger left a comment • edited Loading

Choose a reason for hiding this comment

valpinkman commented Sep 9, 2024

ofreyssinet-ledger left a comment

Choose a reason for hiding this comment

valpinkman commented Sep 4, 2024 •

edited

Loading

vercel bot commented Sep 4, 2024 •

edited

Loading

ofreyssinet-ledger Sep 9, 2024 •

edited

Loading

ofreyssinet-ledger left a comment •

edited

Loading