Case Dismissed: Forcibly show DAppConnectionInfoBar popover on first dApp connection #3464

Shadowfiend · 2023-06-10T19:19:06Z

Draft until #3451 lands.

Adds a DismissableItem concept and related functionality to the
PreferencesService and to the UI slice, providing a common pattern
for things that should be shown to the user exactly once, e.g. new
feature banners and related functions.

This PR will complete the work on updated dApp connections.

Latest build: extension-builds-3464 (as of Fri, 16 Jun 2023 17:51:17 GMT).

background/services/preferences/db.ts

These are items that are displayed to the user either exactly once, or once until the user closes them. The default connection popover is supported, but not yet hooked up. This is designed to be a useful primitive for any new-feature banner, or first-time-use banner. Like many items in preferences, the current list of already-shown dismissable items is initialized when the extension starts, and then updated along the way based on UI actions.

The first connection after a version with the info bar enabled is used will show the popover, then future connections will not. Uses the dismissable item functionality in the preferences service.

The default toggle was being rendered with "Taho as default" being on the left. After some discussion, the fact that it was reversed compared to other toggles (where "on" is on the right) isn't ideal. This commit puts "Taho as default" on the right, and drops the `leftToRight` parameter added to `SharedToggleButton` to make that possible in the first place.

Shadowfiend · 2023-06-15T20:56:16Z

Finally ready to go here I think.

hyphenized

Tested and looks good! Left a few comments

hyphenized · 2023-06-16T05:36:50Z

background/main.ts

+    )
+
+    this.preferenceService.emitter.on(
+      "dismissableItemMarkedAsShown",


I think the benefit to this rather than doing it in the method below that calls markDismissableItemAsShown is that within the preference service we can decide when we dispatch the event and thus update the store, hence we have more flexibility. Though this could turn quite verbose if we use it for everything.

hyphenized · 2023-06-16T05:39:57Z

background/services/preferences/db.ts

+ * be used for tours, or for popups that can be retriggered but will not
+ * auto-display more than once.
+ */
+export type SingleShotItem = "default-connection-popover"


What do you think about OneTimeDismissableItem?

So the distinction I'm trying to draw here, and I think I've done a pretty poor job of it lol, is between items that are dismissed through a user's “please don't show this to me again”-type action, or a banner that needs to be explicitly closed but doesn't block content, and items that are dismissed automatically. I think SingleShotItem is actually a decent name for the latter, but DismissableItem as the umbrella type doesn't reflect that very well.

Maybe it's just AutomaticallyDismissableItem as a counterpart to ManuallyDismissableItem though? 🤔

Ok then I think this is fine as it is 👍

Happy to see/hear ideas for a better name here. Fortunately it's in the type, so it doesn't get persisted, so we can deliberate a little without blocking the merge.

hyphenized · 2023-06-16T05:43:11Z

ui/components/DAppConnection/DAppConnectionInfoBar.tsx

@@ -24,7 +29,7 @@ function DefaultConnectionPopover({ close }: PopoverProps): ReactElement {
  return (
    <div
      className={classNames("bg", {
-        fadeIn: !isClosing,
+        fade_in: !isClosing,


This will break the animation because we haven't made this change yet 👀

Oiiiii how did it sneak innnnnn.

Since this is in the new stuff and I have these changes, how about we keep it and I open the follow-up PR that fixes the fade_in?

Went ahead and dropped this into #3485 .

hyphenized · 2023-06-16T05:49:27Z

ui/components/DAppConnection/DAppConnectionInfoBar.tsx

+  ] = useState(shouldImmediatelyShowPopover)
+
+  const closeDefaultConnectionPopover = useCallback(() => {
+    if (shouldImmediatelyShowPopover && isShowingDefaultConnectionPopover) {


I think we can remove the check for isShowingDefaultConnectionPopover here. Is there a reason we're using useCallback on these functions?

I have to tell you that my radar for when useCallback is the right move and when it's the wrong one is as broken as the tree that was used to make my desk.

Sorry, I missed the first part of the note—why do you think that? It seems like the correct condition, no? If it's not showing and we're set to immediately show it, we shouldn't dispatch anything, because that means it's closing.

Because this only runs when the popover is open right?

I always assume propagation delays for stuff like this, not least because there are cases where we delay rendering changes to wait on animations, etc. We can always case by case those, but it's easier to just consistently get the condition right so reasoning is localized rather than requiring global knowledge of caller behavior.

hyphenized · 2023-06-16T05:53:14Z

background/services/preferences/index.ts

@@ -203,6 +211,8 @@ export default class PreferenceService extends BaseService<Events> {
    )
  }

+  // FIXME This should not be publicly accessible, but triggered by observing
+  // FIXME an event on the signer service.


hyphenized · 2023-06-16T06:06:17Z

background/main.ts

+        // FIXME Both of these should be done as observations of the preference
+        // FIXME service event rather than being managed by `main`.


This pattern seems great but I reckon the logic is going to get a bit hard to follow 🤔. Do we want to only execute calls to other services on events emitted by services as reactions to mutations?

Definitely yes for cross-service stuff. No reason Main should be deciding what the ProviderBridgeService should be doing with an event—that is the ProviderBridgeService's responsibility. The only such triggers from Main should be directly due to redux actions, and in any such action we should be asking “is the service reacting to the user's change, or is the service reacting to a system change”. In this case, the preferences service reacts to the user's change, and that system then becomes a system change that triggers other adjustments. This is just separation of responsibilities, but we haven't done a great job of following it everywhere.

The short articulation is: services are responsible for their own state and for reacting to the state of other services. This is another reason why Main really needs to be renamed to ReduxService; it's charged with mediating with redux, it is NOT some sort of superservice meant to mediate between many services, unless there's some redux-based thing that needs to intervene. Similarly, this is why settings need to make it into PreferencesService—that allows it to be the source of truth for settings, not the redux store, which should generally not be the source of truth for anything.

This opens up the possibility of further paring down the redux store by only storing “recent” things in it, and fetching async from the backend for less recent things (e.g. transactions). I think that's months away, and it won't make sense for everything, but it's what the overall architectural approach is rooted in.

kkosiorowska · 2023-06-21T07:26:15Z

background/services/preferences/db.ts

+    this.version(17).stores({
+      preferences: "++id",
+      signersSettings: "&id",
+      shownDismissableItems: "&id,shown",


Boolean can't be indexed, so adding a shown here does not make much sense.

https://dexie.org/docs/Version/Version.stores()

Bah, yeah. I'd rather not add a migration here but maybe it won't be necessary given we haven't shipped this yet.

@Shadowfiend

## What's Changed * Repeater Connection: Forward requests to new default when default is switched off during dApp connection flow by @Shadowfiend in #3462 * Auto-Not-So-Matic: Fix two matic.network URLs to polygon.technology by @Shadowfiend in #3483 * v0.38.0 by @kkosiorowska in #3480 * Case Dismissed: Forcibly show DAppConnectionInfoBar popover on first dApp connection by @Shadowfiend in #3464 * Add Hardhat Fork Functionality by @0xDaedalus in #3247 * Faded Jeans: Rename fadeIn class to fade_in by @Shadowfiend in #3485 * Fix issue for discovery transaction hash by @kkosiorowska in #3458 * Full Sweep: Drop the USE_UPDATED_SIGNING_UI feature flag by @Shadowfiend in #3475 * Token Discovery - Remap redux asset balances by @hyphenized in #3195 * Make specific warnings for adding custom asset by @kkosiorowska in #3478 * Run NFTs e2e tests on a controlled wallet address by @michalinacienciala in #3487 * v0.38.1 by @jagodarybacka in #3484 **Full Changelog**: v0.38.1...v0.39.0 Latest build: [extension-builds-3496](https://github.com/tahowallet/extension/suites/13792957673/artifacts/764738599) (as of Thu, 22 Jun 2023 14:27:32 GMT).

Shadowfiend commented Jun 12, 2023

View reviewed changes

background/services/preferences/db.ts Outdated Show resolved Hide resolved

Shadowfiend force-pushed the cable-connection branch 2 times, most recently from c7f12f3 to d16131a Compare June 12, 2023 20:27

Shadowfiend mentioned this pull request Jun 12, 2023

Cable Connection: Implement standard flow for dApp connect popup #3451

Merged

6 tasks

Shadowfiend force-pushed the case-dismissed branch 2 times, most recently from 5529ecb to 3940cf6 Compare June 12, 2023 21:07

Base automatically changed from cable-connection to main June 13, 2023 21:38

Shadowfiend added 3 commits June 15, 2023 16:49

Forcibly show DAppConnectionInfoBar popover on first dApp connection

7f8542f

The first connection after a version with the info bar enabled is used will show the popover, then future connections will not. Uses the dismissable item functionality in the preferences service.

Shadowfiend force-pushed the case-dismissed branch from deb4865 to 4c3b7c1 Compare June 15, 2023 20:51

Shadowfiend marked this pull request as ready for review June 15, 2023 20:56

hyphenized reviewed Jun 16, 2023

View reviewed changes

Shadowfiend mentioned this pull request Jun 16, 2023

Faded Jeans: Rename fadeIn class to fade_in #3485

Merged

hyphenized approved these changes Jun 16, 2023

View reviewed changes

hyphenized enabled auto-merge June 16, 2023 17:22

Merge branch 'main' into case-dismissed

cf87049

hyphenized merged commit 1a8e79a into main Jun 16, 2023

hyphenized deleted the case-dismissed branch June 16, 2023 17:51

Shadowfiend mentioned this pull request Jun 20, 2023

Copy to clipboard warning #3488

Merged

2 tasks

kkosiorowska reviewed Jun 21, 2023

View reviewed changes

jagodarybacka mentioned this pull request Jun 22, 2023

v0.39.0 #3496

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Case Dismissed: Forcibly show DAppConnectionInfoBar popover on first dApp connection #3464

Case Dismissed: Forcibly show DAppConnectionInfoBar popover on first dApp connection #3464

Shadowfiend commented Jun 10, 2023 •

edited by github-actions bot

Loading

Shadowfiend commented Jun 15, 2023

hyphenized left a comment

hyphenized Jun 16, 2023

hyphenized Jun 16, 2023

Shadowfiend Jun 16, 2023

hyphenized Jun 16, 2023

Shadowfiend Jun 16, 2023

hyphenized Jun 16, 2023

Shadowfiend Jun 16, 2023

Shadowfiend Jun 16, 2023

hyphenized Jun 16, 2023

Shadowfiend Jun 16, 2023

Shadowfiend Jun 16, 2023

hyphenized Jun 16, 2023

Shadowfiend Jun 21, 2023

hyphenized Jun 16, 2023

hyphenized Jun 16, 2023

Shadowfiend Jun 16, 2023

kkosiorowska Jun 21, 2023

Shadowfiend Jun 21, 2023

		// FIXME Both of these should be done as observations of the preference
		// FIXME service event rather than being managed by `main`.

Case Dismissed: Forcibly show DAppConnectionInfoBar popover on first dApp connection #3464

Case Dismissed: Forcibly show DAppConnectionInfoBar popover on first dApp connection #3464

Conversation

Shadowfiend commented Jun 10, 2023 • edited by github-actions bot Loading

Shadowfiend commented Jun 15, 2023

hyphenized left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shadowfiend commented Jun 10, 2023 •

edited by github-actions bot

Loading