Add indicator for pending proposal #1998

DarksightKellar · 2024-06-13T17:39:19Z

This PR temporarily caches recently created proposal data (transaction hash and metadata). The application can then read this cache and indicate to the user that there are pending proposals waiting to be shown on the UI when ready.

The cached proposal is removed when proposals are loaded and any of them has a transaction hash that matches its transaction hash.

Pending toast:

Toast dismissed when proposal shows up:

Note: the toast is a temporary workaround while waiting on @decentdao/design to finalise the actual look for pending proposal(s) UI.

closes #1983

netlify · 2024-06-13T17:39:37Z

✅ Deploy Preview for decent-interface-dev ready!

Name	Link
🔨 Latest commit	`f81788d`
🔍 Latest deploy log	https://app.netlify.com/sites/decent-interface-dev/deploys/66710585e4951100088131fa
😎 Deploy Preview	https://deploy-preview-1998.app.dev.decentdao.org
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

nicolaus-sherrill · 2024-06-13T18:42:05Z

@DarksightKellar are you thinking that this would be a new card added to the top of the list that communicates "this is temporary until your actual proposal is visible"?

DarksightKellar · 2024-06-13T18:48:05Z

@DarksightKellar are you thinking that this would be a new card added to the top of the list that communicates "this is temporary until your actual proposal is visible"?

@nicolaus-sherrill Yep, I had 2 options in mind:

a single new card that communicates that one or more proposals are pending
a new list that shows basic data for each pending proposal. These won't be clickable, of course

This PR's set up for both, so that won't be an issue, just need to decide which is better UX

Da-Colon

a few minor comments

src/hooks/DAO/loaders/useLoadDAOProposals.ts

src/hooks/DAO/proposal/useSubmitProposal.ts

src/components/Proposals/ProposalsList.tsx

src/hooks/DAO/loaders/useLoadDAOProposals.ts

src/hooks/DAO/proposal/useProposals.ts

adamgall · 2024-06-14T14:31:44Z

src/hooks/DAO/proposal/useProposals.ts

-      .filter(proposal => filters.includes(proposal.state!))
+    return (proposals || [])
+      .filter(proposal => {
+        removeTemporaryProposal(proposal.transactionHash!);


Hmm, at first glance it feels weird to have this state-changing code within a filter....

I wouldn't expect a filter to have side effects like that.

Yeahhh I was trying to avoid having run yet another loop over the proposals just to remove potentially 1 (if ever) pending proposal. Besides filter is just a fancy way of doing map plus manually adding to a pre-declared array based on the predicate. I'm totally fine with it in this case.

I just don't think this code belongs here. We're way to far up at the presentation layer to be fucking around with cleaning up our localstorage state.

Could this go into the the reducer actions for when saving actual proposals into state?

src/hooks/DAO/proposal/useSubmitProposal.ts

adamgall · 2024-06-14T14:35:45Z

src/hooks/DAO/proposal/useSubmitProposal.ts

+      const temporaryProposals = (getValue(CacheKeys.TEMP_PROPOSALS, chain.id) ||
+        []) as TempProposalData[];
+      const updatedProposals = [...temporaryProposals, tempProposal];
+      setValue(CacheKeys.TEMP_PROPOSALS, updatedProposals, CacheExpiry.ONE_DAY);


Hmm, but actually, how was the decision made to store these "temporary pending" proposals in local storage?

I understand that these will persist through a refresh, but do we really need that?

imo seems a bit overkill to utilize localstorage for this functionality.

@DarksightKellar this is probably my most important and biggest-picture question about this PR

Hmm I suppose this could all be implemented with "simpler" app state. But I've tested (and tasted) the ability to refresh and still seeing that persistent pending toast. I like it and don't wanna go back.

Besides, won't the logic largely be the same? The only real difference feels like it'd be where the temp data is coming from (+ the tiny extra ms needed to read/write to storage).

We could go crazy and make it slightly more 'complex' and use netlify/functions and the store? though probably not worth the lift to show all viewers of pending proposals.

It wouldn't make sense to show anyone other than the creator of the proposal, anything about that proposal before their RPC node picked up the block and added it to the official in-memory list.

I do still think localstorage is a bit excessive for this implementation, but that's on me for not being clearer on upfront on what I was expecting.

I'll need to go look at the code again that manages adding/removing from this "pending transaction" datastore in localstorage...

adamgall · 2024-06-14T14:38:41Z

src/hooks/utils/useSafeTransactions.ts

-            transactionHash: multiSigTransaction.transactionHash,
+            transactionHash:
+              multiSigTransaction.transactionHash ||
+              (transaction as SafeMultisigTransactionWithTransfersResponse).safeTxHash,


Is this needed? The code builds for me without this change.

Logically, I suppose the reason why it would needed is if transactionHash is the empty string (Typescript says it's type is simply string).

empty string is falsey, so safeTxHash will be used in the || conditional.

Smells to me like maybe this is an opportunity to tighten up some typing.

Do you follow where my head's at? and if so do you agree? If so, Please add a TODO comment here for someone searching the codebase later for things to do.

It is needed, yep. The typing in that case is misleading, and multiSigTransaction.transactionHash turns out to always be an empty string (or was it undefined?). I chose not to replace the line with transactionHash: (transaction as SafeMultisigTransactionWithTransfersResponse).safeTxHash in case there's some legacy reasoning behind the choice in the first place.

I'll add the todo for the team to revisit this.

src/utils/azorius.ts

tomstuart123 · 2024-06-14T15:39:55Z

@DarksightKellar - I maybe missing something but I couldn't replicate this and get the toaster to show up.

I tried 4 proposals here - https://deploy-preview-1998.app.dev.decentdao.org/proposals?dao=sep:0x40C6E5eFCE3b1f101bFB59e876F14aa5455D3E19

Video here - https://decentdao.slack.com/archives/C06QZTUTKRC/p1718379588738149?thread_ts=1718379569.781799&cid=C06QZTUTKRC

This reverts commit 266653e.

DarksightKellar · 2024-06-14T19:50:40Z

@DarksightKellar - I maybe missing something but I couldn't replicate this and get the toaster to show up.

I tried 4 proposals here - https://deploy-preview-1998.app.dev.decentdao.org/proposals?dao=sep:0x40C6E5eFCE3b1f101bFB59e876F14aa5455D3E19

Video here - https://decentdao.slack.com/archives/C06QZTUTKRC/p1718379588738149?thread_ts=1718379569.781799&cid=C06QZTUTKRC

@tomstuart123 Yikes, one of the newer cleanup commits broke the cache. I've reverted -- should be working now

mudrila

Besides some @adamgall comments - looks good! Straight to prod! 🚀

adamgall · 2024-06-17T02:57:13Z

src/utils/azorius.ts

+  } else {
+    transactionHash = createdEvent.transactionHash;
  }


And then I guess I do have an outstanding question at the moment... what is going on here with finding a new use for this transactionHash property with a brand new createdEvent object?

src/components/pages/DaoDashboard/Activities/index.tsx

src/hooks/DAO/proposal/useProposals.ts

adamgall

Looks good, just a few comments and questions

tomstuart123 · 2024-06-17T12:05:33Z

Awesome, this works so smoothly for 'Create Proposal'. Thanks @DarksightKellar

Last question, pre approval --> which other types of proposals should this work with? I tried it with SendAssets and it worked but not when I tried a change Safename proposal

adamgall · 2024-06-17T13:50:53Z

Awesome, this works so smoothly for 'Create Proposal'. Thanks @DarksightKellar

Last question, pre approval --> which other types of proposals should this work with? I tried it with SendAssets and it worked but not when I tried a change Safename proposal

This is a great question and makes me question whether useSubmitProposal is the appropriate place to trigger adding a temp proposal record...

where else in the codebase can it go to capture all proposal creation, or does that place not exist (yet)?

edit: @tomstuart123 i'm not asking you of course, just thinking out loud

…oposals from localstorage

Need to do more testing

- Reduce as many whitespace changes as possible - add some dev comments to the code in places that look funky - add Pending Proposals to global state - Don't use localstorage for pending data, just in-app memory - Make code to remove "pending proposals" run automatically

adamgall · 2024-06-17T22:57:47Z

ok @decentdao/engineering and @decentdao/product, this is ready for a full code re-review and UX re-test

tomstuart123 · 2024-06-18T15:46:21Z

Nice, this works for all of the below:

Create child safe
Create proposal
send assets
change name
change snapshot space

Approved. Great work

sentry-io · 2024-06-24T22:19:25Z

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

‼️ Error: Proposal state is EXECUTED, but no event found //home View Issue

_{Did you find this useful? React with a 👍 or 👎}

DarksightKellar added 5 commits June 13, 2024 13:11

Cache newly created proposal metadata

13463fc

Indicate number of pending proposals

7b83a4a

Remove temp proposal from cache when actual data comes in

6701a37

Show persistent pending proposal toast

bfdf706

Fix undefined created proposal tx hash bug

2fa0320

DarksightKellar requested review from a team June 13, 2024 17:39

DarksightKellar added 2 commits June 13, 2024 18:40

Tighten up missing proposal created tx hash fix

fc7e37f

cleanup

ba759df

Da-Colon requested changes Jun 14, 2024

View reviewed changes

cleanup

266653e

DarksightKellar requested review from Da-Colon and a team June 14, 2024 14:02

adamgall requested changes Jun 14, 2024

View reviewed changes

DarksightKellar added 9 commits June 14, 2024 20:16

remove unneeded ns

d7a4105

Add pendingProposalNotice translation key

f242199

Add toast to dashboard

52e02b3

Rework pending toast so only one shows at a time

80ce0c0

Revert "cleanup"

ad171b9

This reverts commit 266653e.

Call cacheTemporaryProposal whether successCallback defined or not

bae68c9

add todo

aa8be6c

ActivityBase.transactionHash no longer nullable or undefined-able

2861db8

pretty

6bf3c73

DarksightKellar requested a review from adamgall June 14, 2024 19:59

mudrila approved these changes Jun 15, 2024

View reviewed changes

adamgall reviewed Jun 17, 2024

View reviewed changes

src/components/pages/DaoDashboard/Activities/index.tsx Outdated Show resolved Hide resolved

adamgall reviewed Jun 17, 2024

View reviewed changes

src/hooks/DAO/proposal/useProposals.ts Outdated Show resolved Hide resolved

adamgall requested changes Jun 17, 2024

View reviewed changes

adamgall added 2 commits June 17, 2024 15:15

Clean up some unneeded useRef toastId tracking

7600414

Consolidate logic for showing/hiding toast and removing those temp pr…

7a0c587

…oposals from localstorage

adamgall force-pushed the 1983-indicate-pending-proposal branch 2 times, most recently from b608532 to 7a0c587 Compare June 17, 2024 22:41

adamgall previously approved these changes Jun 17, 2024

View reviewed changes

adamgall self-requested a review June 17, 2024 22:54

Updates:

197a3d8

- Reduce as many whitespace changes as possible - add some dev comments to the code in places that look funky - add Pending Proposals to global state - Don't use localstorage for pending data, just in-app memory - Make code to remove "pending proposals" run automatically

adamgall force-pushed the 1983-indicate-pending-proposal branch from c9f86f1 to 197a3d8 Compare June 17, 2024 22:56

Small copy update

f81788d

adamgall approved these changes Jun 18, 2024

View reviewed changes

adamgall assigned adamgall and DarksightKellar Jun 18, 2024

adamgall force-pushed the 1983-indicate-pending-proposal branch from 08c020a to f81788d Compare June 18, 2024 03:56

tomstuart123 approved these changes Jun 18, 2024

View reviewed changes

Da-Colon approved these changes Jun 18, 2024

View reviewed changes

adamgall merged commit 97b586f into develop Jun 18, 2024
10 checks passed

adamgall deleted the 1983-indicate-pending-proposal branch June 18, 2024 16:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add indicator for pending proposal #1998

Add indicator for pending proposal #1998

DarksightKellar commented Jun 13, 2024

netlify bot commented Jun 13, 2024 •

edited

Loading

nicolaus-sherrill commented Jun 13, 2024

DarksightKellar commented Jun 13, 2024

Da-Colon left a comment

adamgall Jun 14, 2024 •

edited

Loading

DarksightKellar Jun 14, 2024

adamgall Jun 17, 2024

adamgall Jun 14, 2024

adamgall Jun 14, 2024 •

edited

Loading

DarksightKellar Jun 14, 2024

Da-Colon Jun 16, 2024 •

edited

Loading

adamgall Jun 17, 2024

adamgall Jun 17, 2024

adamgall Jun 14, 2024

DarksightKellar Jun 14, 2024

tomstuart123 commented Jun 14, 2024

DarksightKellar commented Jun 14, 2024

mudrila left a comment

adamgall Jun 17, 2024

adamgall left a comment

tomstuart123 commented Jun 17, 2024

adamgall commented Jun 17, 2024 •

edited

Loading

adamgall commented Jun 17, 2024

tomstuart123 commented Jun 18, 2024

sentry-io bot commented Jun 24, 2024

Add indicator for pending proposal #1998

Add indicator for pending proposal #1998

Conversation

DarksightKellar commented Jun 13, 2024

netlify bot commented Jun 13, 2024 • edited Loading

✅ Deploy Preview for decent-interface-dev ready!

nicolaus-sherrill commented Jun 13, 2024

DarksightKellar commented Jun 13, 2024

Da-Colon left a comment

Choose a reason for hiding this comment

adamgall Jun 14, 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

adamgall Jun 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Da-Colon Jun 16, 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

tomstuart123 commented Jun 14, 2024

DarksightKellar commented Jun 14, 2024

mudrila left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamgall left a comment

Choose a reason for hiding this comment

tomstuart123 commented Jun 17, 2024

adamgall commented Jun 17, 2024 • edited Loading

adamgall commented Jun 17, 2024

tomstuart123 commented Jun 18, 2024

sentry-io bot commented Jun 24, 2024

Suspect Issues

netlify bot commented Jun 13, 2024 •

edited

Loading

adamgall Jun 14, 2024 •

edited

Loading

adamgall Jun 14, 2024 •

edited

Loading

Da-Colon Jun 16, 2024 •

edited

Loading

adamgall commented Jun 17, 2024 •

edited

Loading