Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

Dashboard frontend #5241

Merged
merged 74 commits into from
Aug 18, 2022
Merged

Dashboard frontend #5241

merged 74 commits into from
Aug 18, 2022

Conversation

cliffoo
Copy link
Contributor

@cliffoo cliffoo commented Jun 28, 2022

I revamped dashboard frontend.

Why?

This started when I looked into fleshing out features and adding them to dashboard (decoded tx, contract interaction, network + wallet management, and more), and I realized various problems with the existing react code, both the code itself and also more meta (file structure, maintainability) things. So I started from scratch, picked up a component library (mantine) I've been eyeing for a long time, and rebuilt the dashboard frontend.

Some problems and their solutions

Performance
  • Problem

    There are examples of things that cause sub-optimal runtime (take a look at this, ctrl + f "requests" and see how many times it's looping.)

  • Solution

    I've made things as efficient as I could, bearing readability in mind. This specific example above (looping requests) is fixed with a hashmap so constant lookup time.

State management
  • Problem

    State is everywhere right now, it seeps through components at different levels and it's hard to keep track / probably causing extra renders.

  • Solution

    App state is managed better in general. There is a <DashContext /> close to the root that the entire app relies on (to read and mutate state).

UX
  • Problem

    Right now:

    • Wallet is disconnected on refresh.
    • Without having wallet connected, you cannot connect to the dashboard network.
    • The "processing" state for requests is hard-coded. Sometimes on refresh a processed request re-appears.
  • Solution

    • Wallet is only disconnected on explicit user request.
    • While wallet is waiting to be connected, you can connect to the dashboard network and even queue up RPC requests to be processed.
    • Once "Confirm" is clicked the RPC request leaves the frontend state, so it won't show again on refresh (though you can see it on Metamask).
Wagmi
  • Problem

    Wagmi is used weird, it might be because wagmi has evolved and its usage has improved? Take a look at how wagmi is currently provided to the app. I think it's not very readable / might be doing the wrong thing.

  • Solution

    New implementation is clean and easy to understand.

Code organization

This is a big one for me, but it's rather hard to articulate, since there is no right way to organize things in react. I guess an exercise you could do is start from the root (src/App.tsx), dig into how each component relate to each other until you've traversed through all of them; then do the same for this branch and see how they compare.

Here's how I've organized things:

  • App.tsx <- Only context providers and routing go here, no real app logic
  • contexts/
    • DashContext/ <- (Currently single) source for app's state
  • components/
    • common/ <- Reusable components
    • composed/ <- Distinct parts of the app, such as <Sidebar /> and <Txs />, "composed" by nesting smaller components in larger ones, and reusing components from components/common
    • wrappers <- Context providers by 3rd party libs
  • assets/ <- Static things like fonts and images
  • hooks/ <- All custom hooks
  • utils/ <- Convenience things
Craco
  • Problem:

    So it looked like craco was running out of community support, though someone seems to have picked it up recently.

    I don't see a convincing argument for us to use it over CRA. Craco doesn't support CRA 5 yet, we don't need to override any webpack configs, and we're currently only using craco to get tailwind working (tailwind is no longer needed, as mantine uses emotion for styling).

  • Solution:

    CRA.

Eslint & preflight checks
Security

I'm probably being more cautious than necessary here, but since a big reason why you'd use dashboard in the first place is security (no need to hand off mnemonic, just let Metamask take care of it), as I mentioned here I want to minimize / elimiate all network requests that are not used for processing RPC requests.

This means no more requests to the following:

  • Google fonts cdn (fonts bundled now)
  • chainid.network/chains.json (chain info bundled also)

Other things

  • Re-renders are minimized. Lifecycles and states are efficiently managed.
  • import / export type wherever possible. This is good because type imports are fully erased during runtime.
  • Dev deps that should be deps, are now deps.

Fun things

  • Dark mode.
  • Smoother experience in general.

Why this is good

The biggest win for me is that this sets up a good foundation for future improvements. There is a clearer path forward on integrating new features, reasons in the lists above.

How to review

The best way is probably to ignore how dashboard frontend is now. I.e. When going through the changes, ignore all deletions (red) and look only at additions (green). There is almost no inherited code.

Problems I've introduced

  • [Fixed by upgrading to mantine 5] Mantine + react dependency clash. See this.
  • [Fixed by upgrading to mantine 5] flushSync warning on clipboard copy in modal. See this.
  • When lerna links dependencies
    • [Fixed by adding necessary babel plugins and core] Babel peer deps warning, but things work ok?
    • React native peer deps warning, but no I'm not adding that.

Issues that are conveniently resolved

  • Blank page issue#5050 => This shouldn't happen, unless there's no browser wallet (like Metamask). (Aside: I'll probably add an ErrorBoundary in the future to catch errors like no wallet found.)
  • Bad chain names issue#5020 => 1337 resolves to "Dev Network" now.
  • Hard-coded processing state issue#4982 => Once tx / call is processed it is removed from dashboard state. You won't see a processed request on refresh.
  • Chain names caching issue#4949 => No need to cache, chain names are now bundled.
  • Outdated react-scripts issue#4874 => Updated to latest.

There may be more? I just searched is:issue is:open dashboard and filtered through the result.

Future

This PR was originally more ambitious, but in favor of better git practices and PR reviewers' time, new features will ensue in the form of smaller, more digestible PRs after this one merges against develop.

Progress

  • Bomb
  • Clear outdated caniuse-lite warning (already made it to develop)
  • Add and setup mantine
  • Add and setup react-router-dom
  • Layout and nav
  • Wagmi context
  • Basic network management
    • (Dis)connect
    • Display chain name + id
  • <DashContext />
    • Setup
    • Message bus client setup
    • Notice system
    • Wire up wagmi and message bus client
    • Chain name
  • Message queue
  • Message modal
  • Confirm network (initially and on switch)
  • Make things look nice
  • Clean up commit messages
  • Rebase
  • Mantine 5 came out very recently, upgrade
  • Fix ci

@cliffoo cliffoo closed this Jun 28, 2022
@cliffoo cliffoo reopened this Jul 1, 2022
@cliffoo cliffoo force-pushed the dash-woah branch 4 times, most recently from 13c268a to da4f050 Compare July 6, 2022 05:01
@cliffoo cliffoo self-assigned this Jul 7, 2022
@cliffoo cliffoo changed the title WIP: Dashboard improvements WIP: Dashboard frontend redo Jul 8, 2022
@cliffoo cliffoo mentioned this pull request Jul 9, 2022
1 task
@cliffoo cliffoo force-pushed the dash-woah branch 2 times, most recently from 17e1749 to f35c516 Compare July 13, 2022 07:51
@cliffoo cliffoo force-pushed the dash-woah branch 4 times, most recently from 48c43f4 to 3467c5e Compare July 15, 2022 18:27
@cliffoo cliffoo force-pushed the dash-woah branch 9 times, most recently from 782a6f6 to e0e58d1 Compare July 26, 2022 20:28
@cliffoo cliffoo marked this pull request as ready for review July 26, 2022 21:02
Cliff Zhang and others added 26 commits August 18, 2022 09:10
Stop the cap

Co-authored-by: g. nicholas d'andrea <[email protected]>
Bad nullish coalesce usage
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants