Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace toasts with improved custom optimistic UX #1161

Closed
wants to merge 56 commits into from
Closed

Conversation

ekzyis
Copy link
Member

@ekzyis ekzyis commented May 8, 2024

Description

Close #849 Related to #848

As mentioned in #1071, this replaces useInvoiceable with usePayment which has a simpler API.

This PR also contains changes to improve the external payment UX by replacing generic toasts with custom UX code per action.

TODO:

  • Replace useInvoiceable with usePayment #1071
  • optimistic zap UX
  • optimistic custom zap UX (custom zap = zapping via long press)
  • optimistic reply UX
  • optimistic post UX canceled because this is most likely better implemented using server-side code so let's first get user feedback on client-side optimistic update code before delving deeper into UX patterns that include server-side code
    • bounty form
    • discussion form
    • link form
    • poll form
    • job form
  • optimistic poll votes UX
  • optimistic bounty payments UX
  • territory admin optimistic UX
    • create
    • update
    • unarchive
  • (zap) undos
  • remove unnecessary payment context (see this comment)
  • fix meAnonSats not immediately updated

Error handling

  • inform users about errors in notifications to let them retry
  • remove fallback to QR code on attached wallet payment failure since it interrupts UX (rely on out-of-band error handling like notifications instead) (?) let's see in production first
  • fix Cannot read properties of null (reading 'meSats') when reloading page on /notifications and trying to zap error notification because cache is not populated
  • fix client notifications don't use cache but static item data. this means optimistic updates don't work for client notifications.

Screenshots

Since toasts for success and error were removed for better UX, we now use dismissible error notifications that are only stored locally. So if you zap something and it fails, it looks like this:

localhost_3000_(iPhone SE)

Additional Context

See #1071 (comment) for additional context regarding move from useInvoiceable to usePayment. Most notably, JITInvoice is no longer with us. I removed it because it made the code more complicated and the retry/cancel UX was bad and lead regularly to confusion ("sats received" vs "error"). The HODL invoice is now immediately canceled if the action fails. With the new custom optimistic UX, retries via literally retrying (pressing reply, zapping again etc.) are imo simple enough.

Checklist

Are your changes backwards compatible? Please answer below:

Did you QA this? Could we deploy this straight to production? Please answer below:

For frontend changes: Tested on mobile? Please answer below:

Did you introduce any new environment variables? If so, call them out explicitly here:

@ekzyis ekzyis added the ux label May 8, 2024
Copy link
Contributor

coderabbitai bot commented May 8, 2024

Walkthrough

The modifications involve a series of enhancements and refactorings across various components, emphasizing payment handling, error management, and user experience improvements in response handling. Key changes include the introduction of new payment and error handling utilities, modifications to form and button components to integrate these utilities, and updates to the UI components to reflect pending states and improve interaction feedback.

Changes

Files Change Summary
api/resolvers/item.js, components/payment.js Enhanced payment integration and error handling.
components/bounty-form.js, components/job-form.js Split invoiceable into separate props, wrapped components in PaymentProvider.
components/item-act.js, components/pay-bounty.js Simplified payment logic, removed old payment hooks, integrated new payment handling.
components/fee-button.js, components/upvote.js Added payment status feedback in UI components.
components/item-info.js, components/invoice.js Enhanced error handling and data fetching, added pending state management.
components/toast.js, lib/new-comments.js Streamlined toast management, modified local storage handling in comments functionality.

Assessment against linked issues

Objective Addressed Explanation
Enable UI to update instantly when external payments are made (#848)
Handle scenarios where payments fail by providing a notification for retry (#848)
Introduce new columns (invoiceId and status) in the actions table to track payment status (#848) The PR focuses on frontend changes; backend schema modifications are not included.
Display pending actions as if they are settled for the user who initiated the action (#848)
Improve non-custodial UX for cancellations and retries beyond generic toast messages (#849) While the PR improves feedback mechanisms, it's unclear if it fully addresses the refined UX needs.

Possibly related issues


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@ekzyis ekzyis changed the title Replace toasts with improved optimistic UX Replace toasts with improved custom optimistic UX May 8, 2024
@ekzyis ekzyis marked this pull request as draft May 8, 2024 02:59
components/payment.js Outdated Show resolved Hide resolved
@ekzyis ekzyis force-pushed the wallet-ux branch 6 times, most recently from 2de9cd1 to 4b083ab Compare May 9, 2024 10:29
@ekzyis
Copy link
Member Author

ekzyis commented May 9, 2024

Random better idea for zap undos: instead of this ugly orange toast with "undo" written inside, use the pending pulse animation I had before and use that as undo.

So while the bolt is pulsing, you can click it again to undo.

But if you're too slow, you'll zap your default, lol.

I think @notnout mentioned this idea for undos first.

update: Implemented in 48c0142

@ekzyis ekzyis force-pushed the wallet-ux branch 2 times, most recently from 8ca84df to f829627 Compare May 10, 2024 19:43
@ekzyis ekzyis marked this pull request as ready for review May 11, 2024 12:17
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 9

Out of diff range and nitpick comments (6)
components/webln/nwc.js (2)

210-214: Optimize the timeout handling in the sendPayment function to align with best practices.

Consider using a named function for the timeout callback to improve readability and potential reusability.


210-214: Enhance the robustness of the payment process by adding more detailed logging and error information.

Consider adding more detailed logs and error information to help with debugging and maintaining the payment process.

components/reply.js (1)

270-280: Enhance the robustness of the reply process by adding more detailed logging and error information.

Consider adding more detailed logs and error information to help with debugging and maintaining the reply process, especially in error scenarios.

components/item-act.js (1)

104-116: Enhance the robustness of the action process by adding more detailed logging and error information.

Consider adding more detailed logs and error information to help with debugging and maintaining the action process, especially in error scenarios.

pages/settings/index.js (1)

29-29: Consider adding a comment explaining the purpose of INVOICE_RETENTION_DAYS and ZAP_UNDO_DELAY.

Adding comments for these constants can improve code readability and maintainability, especially for new developers or when revisiting this section after a long time.

components/form.js (1)

Line range hint 805-868: Refactor the Form component to integrate new hooks and error handling.

-  const payment = usePayment()
-  const me = useMe()
+  const { request } = usePayment()
+  const { user } = useMe()

Ensure destructuring is used for consistency and to avoid potential bugs related to direct property access on hook return values.

Comment on lines +836 to +868
const onSubmitInner = useCallback(async ({ amount, ...values }, ...args) => {
let cancel, revert, beforeSubmitReturn
try {
if (onSubmit) {
// extract cost from formik fields
// (cost may also be set in a formik field named 'amount')
const cost = feeButton?.total || values?.amount
if (cost) {
values.cost = cost
if (requireSession && !me) {
throw new SessionRequiredError()
}
beforeSubmitReturn = beforeSubmit?.({ amount, ...values })
let hash, hmac
if (invoiceable) {
revert = await optimisticUpdate?.({ amount, ...values }, ...args);
[{ hash, hmac }, cancel] = await payment.request(amount)
}
await onSubmit(values, ...args)
await onSubmit({ hash, hmac, amount, ...values }, ...args)
if (!storageKeyPrefix) return
clearLocalStorage(values)
}
} catch (err) {
revert?.()
if (err instanceof InvoiceCanceledError || err instanceof ActCanceledError) {
return
}
const msg = err.message || err.toString?.()
// ignore errors from JIT invoices or payments from attached wallets
// that mean that submit failed because user aborted the payment
if (msg === 'modal closed' || msg === 'invoice canceled') return
toaster.danger('submit error: ' + msg)
if (onError) {
onError({ amount, ...values, reason: msg })
} else {
toaster.danger('submit error: ' + msg)
}
cancel?.()
} finally {
afterSubmit?.({ amount, ...values, ...beforeSubmitReturn })
}
}, [onSubmit, feeButton?.total, toaster, clearLocalStorage, storageKeyPrefix])
}, [me, onSubmit, feeButton?.total, toaster, clearLocalStorage, storageKeyPrefix, payment])
Copy link
Contributor

Choose a reason for hiding this comment

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

Enhance the onSubmitInner function with session checks and payment handling.

-  if (requireSession && !me) {
+  if (requireSession && !user) {
     throw new SessionRequiredError()
   }
-  [{ hash, hmac }, cancel] = await payment.request(amount)
+  const { hash, hmac, cancel } = await request(amount)

Refactor to use destructuring for clarity and to handle the case where payment.request might return an object instead of an array.


Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
const onSubmitInner = useCallback(async ({ amount, ...values }, ...args) => {
let cancel, revert, beforeSubmitReturn
try {
if (onSubmit) {
// extract cost from formik fields
// (cost may also be set in a formik field named 'amount')
const cost = feeButton?.total || values?.amount
if (cost) {
values.cost = cost
if (requireSession && !me) {
throw new SessionRequiredError()
}
beforeSubmitReturn = beforeSubmit?.({ amount, ...values })
let hash, hmac
if (invoiceable) {
revert = await optimisticUpdate?.({ amount, ...values }, ...args);
[{ hash, hmac }, cancel] = await payment.request(amount)
}
await onSubmit(values, ...args)
await onSubmit({ hash, hmac, amount, ...values }, ...args)
if (!storageKeyPrefix) return
clearLocalStorage(values)
}
} catch (err) {
revert?.()
if (err instanceof InvoiceCanceledError || err instanceof ActCanceledError) {
return
}
const msg = err.message || err.toString?.()
// ignore errors from JIT invoices or payments from attached wallets
// that mean that submit failed because user aborted the payment
if (msg === 'modal closed' || msg === 'invoice canceled') return
toaster.danger('submit error: ' + msg)
if (onError) {
onError({ amount, ...values, reason: msg })
} else {
toaster.danger('submit error: ' + msg)
}
cancel?.()
} finally {
afterSubmit?.({ amount, ...values, ...beforeSubmitReturn })
}
}, [onSubmit, feeButton?.total, toaster, clearLocalStorage, storageKeyPrefix])
}, [me, onSubmit, feeButton?.total, toaster, clearLocalStorage, storageKeyPrefix, payment])
const onSubmitInner = useCallback(async ({ amount, ...values }, ...args) => {
let cancel, revert, beforeSubmitReturn
try {
if (onSubmit) {
if (requireSession && !user) {
throw new SessionRequiredError()
}
beforeSubmitReturn = beforeSubmit?.({ amount, ...values })
let hash, hmac
if (invoiceable) {
revert = await optimisticUpdate?.({ amount, ...values }, ...args);
const { hash, hmac, cancel } = await request(amount)
}
await onSubmit({ hash, hmac, amount, ...values }, ...args)
if (!storageKeyPrefix) return
clearLocalStorage(values)
}
} catch (err) {
revert?.()
if (err instanceof InvoiceCanceledError || err instanceof ActCanceledError) {
return
}
const msg = err.message || err.toString?.()
if (onError) {
onError({ amount, ...values, reason: msg })
} else {
toaster.danger('submit error: ' + msg)
}
cancel?.()
} finally {
afterSubmit?.({ amount, ...values, ...beforeSubmitReturn })
}
}, [me, onSubmit, feeButton?.total, toaster, clearLocalStorage, storageKeyPrefix, payment])

Comment on lines +210 to +214
const timeout = JIT_INVOICE_TIMEOUT_MS
const timer = setTimeout(() => {
const msg = 'timeout waiting for info event'
const msg = 'timeout waiting for payment'
logger.error(msg)
reject(new Error(msg))
reject(new InvoiceExpiredError())
Copy link
Contributor

Choose a reason for hiding this comment

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

Refactor the error handling in the sendPayment function to ensure consistency and clarity.

-          const timeout = JIT_INVOICE_TIMEOUT_MS
-          const timer = setTimeout(() => {
-            const msg = 'timeout waiting for payment'
-            logger.error(msg)
-            reject(new InvoiceExpiredError())
+          const timer = setTimeout(() => {
+            logger.error('timeout waiting for payment')
+            reject(new InvoiceExpiredError('Payment timeout reached'))

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
const timeout = JIT_INVOICE_TIMEOUT_MS
const timer = setTimeout(() => {
const msg = 'timeout waiting for info event'
const msg = 'timeout waiting for payment'
logger.error(msg)
reject(new Error(msg))
reject(new InvoiceExpiredError())
const timer = setTimeout(() => {
logger.error('timeout waiting for payment')
reject(new InvoiceExpiredError('Payment timeout reached'))

Comment on lines +21 to +35
const cacheAddComment = (cache, parentId, data) => {
cache.modify({
id: `Item:${parentId}`,
fields: {
comments (existingCommentRefs = []) {
const newCommentRef = cache.writeFragment({
data,
fragment: COMMENTS,
fragmentName: 'CommentsRecursive'
})
return [newCommentRef, ...existingCommentRefs]
}
}
})
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Refactor the cacheAddComment function to improve readability and maintainability.

-  cache.modify({
-    id: `Item:${parentId}`,
-    fields: {
-      comments (existingCommentRefs = []) {
-        const newCommentRef = cache.writeFragment({
-          data,
-          fragment: COMMENTS,
-          fragmentName: 'CommentsRecursive'
-        })
-        return [newCommentRef, ...existingCommentRefs]
-      }
-    }
-  })
+  const newCommentRef = cache.writeFragment({
+    data,
+    fragment: COMMENTS,
+    fragmentName: 'CommentsRecursive'
+  })
+  cache.modify({
+    id: `Item:${parentId}`,
+    fields: {
+      comments (existingCommentRefs = []) {
+        return [newCommentRef, ...existingCommentRefs]
+      }
+    }
+  })

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
const cacheAddComment = (cache, parentId, data) => {
cache.modify({
id: `Item:${parentId}`,
fields: {
comments (existingCommentRefs = []) {
const newCommentRef = cache.writeFragment({
data,
fragment: COMMENTS,
fragmentName: 'CommentsRecursive'
})
return [newCommentRef, ...existingCommentRefs]
}
}
})
}
const cacheAddComment = (cache, parentId, data) => {
const newCommentRef = cache.writeFragment({
data,
fragment: COMMENTS,
fragmentName: 'CommentsRecursive'
})
cache.modify({
id: `Item:${parentId}`,
fields: {
comments (existingCommentRefs = []) {
return [newCommentRef, ...existingCommentRefs]
}
}
})
}

Comment on lines +176 to +183
// prevent edit timer jump
const pendingComment = cacheRemovePendingComment(cache, parentId)
const comment = { ...upsertComment, createdAt: pendingComment.createdAt }
cacheAddComment(cache, parentId, comment)

const ancestors = item.path.split('.')

// update all ancestors
ancestors.forEach(id => {
cache.modify({
id: `Item:${id}`,
fields: {
ncomments (existingNComments = 0) {
return existingNComments + 1
}
}
})
})
// XXX cache update already applied
// cacheUpdateAncestors(cache, ancestors)
Copy link
Contributor

Choose a reason for hiding this comment

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

Optimize the reply submission and cache update logic to enhance performance and clarity.

Consider restructuring the logic to separate concerns more clearly, potentially splitting the function into smaller, more focused functions.

Comment on lines +210 to +214
const optimisticUpdate = useCallback(({ text }, { resetForm }) => {
setReply(replyOpen || false)
resetForm({ text: '' })
return upsertCommentOptimisticUpdate(cache, { ...item, text }, { me })
}, [setReply, cache, item, me])
Copy link
Contributor

Choose a reason for hiding this comment

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

Refine the optimistic update logic to ensure it aligns with the overall application architecture and error handling strategy.

Consider reviewing and potentially refactoring the optimistic update logic to ensure it integrates seamlessly with the rest of the application, particularly in terms of error handling and state management.

components/item-act.js Show resolved Hide resolved
Comment on lines +140 to +186
const updateItemSats = (cache, { id, path, act, sats }, { me }) => {
if (sats < 0) {
// if sats < 0, we are reverting.
// in that case, it is important that we first revert the persisted sats
// before calling cache.modify since cache.modify will trigger field reads
// and thus persisted sats will be counted
if (!me) persistItemAnonSats({ id, path, act, sats })
else persistItemPendingSats({ id, path, act, sats })
}

const update = useCallback((cache, args) => {
const { data: { act: { id, sats, path, act } } } = args
cache.modify({
id: `Item:${id}`,
fields: {
sats (existingSats = 0) {
return act === 'TIP' ? existingSats + sats : existingSats
},
meSats (existingSats = 0) {
return act === 'TIP' ? existingSats + sats : existingSats
},
meDontLikeSats: me
? (existingSats = 0) => {
return act === 'DONT_LIKE_THIS' ? existingSats + sats : existingSats
}
: undefined
}
})

cache.modify({
id: `Item:${id}`,
fields: {
sats (existingSats = 0) {
if (act === 'TIP') {
return existingSats + sats
if (act === 'TIP') {
// update all ancestors
path.split('.').forEach(aId => {
if (Number(aId) === Number(id)) return
cache.modify({
id: `Item:${aId}`,
fields: {
commentSats (existingCommentSats = 0) {
return existingCommentSats + sats
}
}
})
})
}

return existingSats
},
meSats: me
? (existingSats = 0) => {
if (act === 'TIP') {
return existingSats + sats
}
if (sats > 0) {
if (!me) persistItemAnonSats({ id, path, act, sats })
else persistItemPendingSats({ id, path, act, sats })
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Optimize the action submission and cache update logic to enhance performance and clarity.

Consider restructuring the logic to separate concerns more clearly, potentially splitting the function into smaller, more focused functions.

Comment on lines +78 to +94
const optimisticUpdate = useCallback(async ({ amount }) => {
onClose()
strike()
const revert = actOptimisticUpdate(cache, { ...item, sats: Number(amount), act: down ? 'DONT_LIKE_THIS' : 'TIP' }, { me })
abortSignal?.start()
if (zapUndoTrigger(me, amount)) {
try {
await zapUndo(abortSignal)
} catch (err) {
revert()
throw err
}
} else {
abortSignal?.done()
}
)
return revert
}, [cache, strike, onClose, abortSignal])
Copy link
Contributor

Choose a reason for hiding this comment

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

Refine the optimistic update logic to ensure it aligns with the overall application architecture and error handling strategy.

Consider reviewing and potentially refactoring the optimistic update logic to ensure it integrates seamlessly with the rest of the application, particularly in terms of error handling and state management.

Comment on lines +304 to +299
revert?.()
if (error instanceof InvoiceCanceledError || error instanceof ActCanceledError) {
return
}
console.error(error)
toaster.danger('zap: ' + error?.message || error?.toString?.())
const reason = error?.message || error?.toString?.()
if (me) notify(NotificationType.ZapError, { reason, amount: sats - meSats, itemId: item.id })
else toaster.danger('zap error: ' + reason)
cancel?.()
} finally {
Copy link
Contributor

Choose a reason for hiding this comment

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

Enhance the error handling in the action functionality to ensure robustness and clarity.

Consider refining the error handling logic to ensure it is consistent and clear, potentially adding more detailed error messages and handling specific error types more effectively.

Comment on lines +606 to +609
const filtered = JSON.parse(stored).filter(({ sortTime }) => {
// only keep notifications younger than 24 hours
return new Date(sortTime) >= datePivot(new Date(), { hours: -24 })
})
Copy link
Member Author

Choose a reason for hiding this comment

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

Only keep notifications younger than 24 hours

This helps with not bloating /notifications and local storage.

It also is a workaround for having to come up with some complicated pagination algorithm which considers how many / which client notifications should be included on each page.

-- 5ff6210

I think this is smart lazy stuff

Copy link
Member

Choose a reason for hiding this comment

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

This is a reasonable shim for sure

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed the local field item.meAnonSats because I realized I can simply add them to item.meSats via the read function

@@ -1,5 +1,6 @@
.toastContainer {
transform: translate3d(0, 0, 0);
margin-bottom: 53px; /* padding for bottom nav bar on mobile */
Copy link
Member Author

Choose a reason for hiding this comment

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

unrelated fix for toasts shadowing bottom nav bar

Copy link
Member

@huumn huumn left a comment

Choose a reason for hiding this comment

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

This looks really thoughtfully done on first pass. I'll do a deep review and QA the first chance I get tomorrow.

Comment on lines 32 to 33
onClose={(amount) => {
onClose()
Copy link
Member

Choose a reason for hiding this comment

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

nit but onClose={onClose}?

Copy link
Member Author

Choose a reason for hiding this comment

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

Huch, didn't notice this on my crusade against old zap undo code. Good catch

}
}, [baseLineItems, lineItems, remoteLineItems, mergeLineItems, disabled, setDisabled])
}, [me, baseLineItems, lineItems, remoteLineItems, mergeLineItems, disabled, setDisabled])
Copy link
Member

Choose a reason for hiding this comment

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

nit but this will rerender every second because me updates on poll

Suggested change
}, [me, baseLineItems, lineItems, remoteLineItems, mergeLineItems, disabled, setDisabled])
}, [me?.privates?.sats, baseLineItems, lineItems, remoteLineItems, mergeLineItems, disabled, setDisabled])

Comment on lines +38 to +43
export class SessionRequiredError extends Error {
constructor () {
super('session required')
this.name = 'SessionRequiredError'
}
}
Copy link
Member

Choose a reason for hiding this comment

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

So that's how you create a custom javascript error! 😆

}
await onSubmit(values, ...args)
await onSubmit({ hash, hmac, amount, ...values }, ...args)
Copy link
Member

Choose a reason for hiding this comment

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

This changes the signature of onSubmit. Does anything use args?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm only adding hash, hmac, amount to values which was already an object. I don't think this changes the signature.

You might missed the curly braces around the comma separated values?

Copy link
Member

Choose a reason for hiding this comment

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

Oh right, values was already an object. What's in args?

Copy link
Member Author

Choose a reason for hiding this comment

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

formik helper stuff like resetForm. They call it "formikBag" in the documentation. Maybe we should use that name, too. Since we no longer wrap submit handlers for our payment flow, I think args is really the same as formikBag.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see. I remember using resetForm at one point.

Someone at Square told me they don't allow engineers to declare array indices as i. Kind of intense lol

@huumn
Copy link
Member

huumn commented May 13, 2024

Somehow I end up increasing my balance with this approach. Aren't those invoices not supposed to settle?

Also, one of my zaps ended up showing up as a 0-sat flag (on the sn wallet plans post). No idea what that's about, but I suspect there's a bug somewhere that caused that.

Did you test all sending side wallets after these changes? It might help discover more unhappy paths.

Screen.Recording.2024-05-13.at.10.52.30.AM.mp4

After I stopped recording this video my balance went up to 200 sats.

Copy link
Member

@huumn huumn left a comment

Choose a reason for hiding this comment

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

Failing QA

@ekzyis ekzyis marked this pull request as draft May 13, 2024 21:34
@ekzyis
Copy link
Member Author

ekzyis commented May 13, 2024

Somehow I end up increasing my balance with this approach. Aren't those invoices not supposed to settle?

You're right, they should either settle because they were consumed by an action or canceled. So in both cases, the balance shouldn't increase.

Also, one of my zaps ended up showing up as a 0-sat flag (on the sn wallet plans post). No idea what that's about, but I suspect there's a bug somewhere that caused that.

I had this error before. It happens when a payment fails and the code that reverts the optimistic update ends up writing a negative value instead of the value before the update was done. I thought I fixed it 🤔

Did you test all sending side wallets after these changes? It might help discover more unhappy paths.

No, I assumed that all sending wallets would behave the same since I only relied on sendPayment working afaict. But I guess there are some details I missed, most likely related to error handling.

@huumn
Copy link
Member

huumn commented May 13, 2024

No, I assumed that all sending wallets would behave the same since I only relied on sendPayment working afaict. But I guess there are some details I missed, most likely related to error handling.

Kind of meta, but because all the wallets are all in a beta state, we should probably QA all of them when we make changes to related code. It'll also help us familiarize ourselves with the UX of each wallet over and over again, so that all of them work equally well. So it's both QA and UX investigation.

This reminds me, I need to get NWC working locally and add a LNBits container.

@ekzyis
Copy link
Member Author

ekzyis commented May 13, 2024

I need to get NWC working

#1151 should have you covered. You only need to install nostr-wallet-connect-lnd. It uses wss://relay.damus.io by default which has rate limits which can be annoying. But so far, it wasn't annoying enough to find a solution.

add a LNBits container.

Yep, also thought about this. I'll do this today. I think this is more in my ball park anyway since I also use LNbits to manage my lightning credentials for hnbot and oracle + I need this to test this PR.

@ekzyis ekzyis force-pushed the wallet-ux branch 2 times, most recently from 81d7805 to 3ddd6b4 Compare May 15, 2024 09:49
@ekzyis
Copy link
Member Author

ekzyis commented May 15, 2024

Mhh, I fixed a race condition in 3ddd6b4 but there is still one: when we zap too fast, pending sats are counted again for each zap since cache.modify triggers readField which then adds pending sats that are already included in existing sats again to the existing sats.

For some reason I completely missed this before, I must have been too cautious in testing my code or NWC payments were too slow so I didn't notice this. I tried to fix this by accounting for existing pending sats when calling cache.modify but didn't work for some reason. So I guess there is more stuff broken in my assumptions.

This also isn't the only issue I found: We don't update me.privates.sats so there is also a bug where following zaps reuse the existing balance and thus think they don't need an invoice but then fail since they actually do.

I'm starting to believe we should straight go for storing pending states in the server and skip this release of client-side optimistic updates. On the server, we can use database transactions to make sure we don't run into race conditions like this. Feels bad that it took me so long to realize that doing this on the client is way too messy and buggy but better pivoting late than releasing this in its current state. Going to sleep over this but I think this is the right call. Doing all of this stuff on the server should be much simpler, thus cleaner and is what we wanted to do anyway.

ekzyis added 26 commits May 15, 2024 18:45
This also means we can use item links as for other notifications since they weren't added to client notifications to make the dismissal button work properly.
This helps with not bloating /notifications and local storage.

It also is a workaround for having to come up with some complicated pagination algorithm which considers how many / which client notifications should be included on each page.
By populating n.item during render, we no longer use stale item data that was set during notify().

This also fixes following issues:

* the need to add item.root manually to n.item
* the need to populate the cache such that readFragment during optimistic updates finds item fragments
If zapping too fast, item.meSats inside optimistic update could include pending sats from previous zap unlike item.meSats in calling context.
This is fixed by passing satsDelta instead of calculating it independently with readFragment.
This race condition lead to following unexpected behavior that is now fixed:

* pending sats could go negative causing items to falsely be marked as flagged

unrelated fix:

* fix zapUndoTrigger using total sats not satsDelta
@ekzyis
Copy link
Member Author

ekzyis commented May 18, 2024

Superseded by #1184

@ekzyis ekzyis closed this May 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

External pending payment UX
2 participants