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

Creation and Submission delay and status #457

Open
wants to merge 14 commits into
base: dev
Choose a base branch
from

Conversation

ticruz38
Copy link
Collaborator

@ticruz38 ticruz38 commented Oct 31, 2024

This PR address issues #369 and #235

  • New setting to delay the publishing of a note (default is 0)
  • Cancelable toast when creating a new note
  • Inline Note status when replying to a note.

When replying, there is a (configurable) 5 seconds countdown before the note is sent
Screenshot 2024-10-31 at 16 40 06

Once the note is sent, it tracks the pending requests via a loading bar
Screenshot 2024-10-31 at 16 40 12

Once all request are settled, it shows a status summary, with the possibility to open a more detailed modal
Screenshot 2024-10-31 at 16 40 25

The post status is displayed for 30 seconds, after which the actions buttons shows up like a normal note
Screenshot 2024-10-31 at 16 40 53

@ticruz38 ticruz38 changed the base branch from master to dev October 31, 2024 15:56
@ticruz38 ticruz38 marked this pull request as ready for review October 31, 2024 15:56
@ticruz38 ticruz38 requested a review from staab October 31, 2024 15:56
@staab
Copy link
Collaborator

staab commented Oct 31, 2024

This diff is wonky too. For some reason it's showing all my commits as your commits. I don't know what you're doing to make that happen, but can you reset/rebase to clean it up?

Copy link
Collaborator

@staab staab left a comment

Choose a reason for hiding this comment

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

You'll need to rebase this on dev since I merged the groups stuff. Also, take a look at @welshman/app's thunk module, it solves a lot of these problems much more cleanly. I'm not sure if we can integrate it cleanly yet, but see if you run into any problems with that.

src/app/shared/Note.svelte Outdated Show resolved Hide resolved
src/util/misc.ts Outdated Show resolved Hide resolved
src/app/shared/Note.svelte Outdated Show resolved Hide resolved
@ticruz38
Copy link
Collaborator Author

ticruz38 commented Nov 6, 2024

thunk module, it solves a lot of these problems much more cleanly. I'm not sure if we can integrate it cleanly yet, but see if you run into any problems with that.

So I tried, for the NoteCreate, it's ok, I can watch the thunk status and take action depending on it (clear the modal, show the toaster etc).
EDIT: No I need a callback, the thunk status or result only resolved once the relay replied, I need either one more status (SENT?) or a callback.

The canceled is handled with thunk.controller.abort(). Which is cleaner than keeping a cancel state in the component.

For the reply, it's feasible too but the thunk will have to travel across 3 or 4 components like this.
NoteReply > Note(parent) > Note(reply) > NotePending

Atm I am using the publish command that populates the $publishes store, I can then import directly in NotePending and check out the state of my pub. It seems cleaner.

@staab
Copy link
Collaborator

staab commented Nov 7, 2024

EDIT: No I need a callback, the thunk status or result only resolved once the relay replied, I need either one more status (SENT?) or a callback.

Look for the Pending status, that gets set when the event is first sent to the relay. If pub.status is empty, that means the event hasn't been sent yet. You should also be able to pass delay to the thunk to handle the timeout.

For the reply, it's feasible too but the thunk will have to travel across 3 or 4 components like this.
NoteReply > Note(parent) > Note(reply) > NotePending

I'm actually hoping to migrate from publishes to thunks (which are a superset of the functionality), this issue is actually a step in that migration. We can just add these to engine/state so they're available from anywhere. If you want to go ahead and do that migration as part of this issue, and get rid of publishes entirely, go ahead, they're only used in a couple places.

@ticruz38
Copy link
Collaborator Author

ticruz38 commented Nov 8, 2024

ok I'll make the migration to thunk, almost there

@ticruz38
Copy link
Collaborator Author

Just finished the thunk migration and rebased from dev

Might be worth another look

Copy link
Collaborator

@staab staab left a comment

Choose a reason for hiding this comment

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

When delay = 0, the tooltip still shows while the remote signer responds:

Screenshot 2024-11-12 at 4 07 58 PM

Not sure why, but a duplicate thunk is getting added with an undefined ID:
Screenshot 2024-11-12 at 4 10 08 PM

@@ -1 +1 @@
npm run check
npm run format && npm run check
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
npm run format && npm run check
npm run check

src/app/views/UserSettings.svelte Outdated Show resolved Hide resolved
src/partials/Toast.svelte Show resolved Hide resolved
src/partials/Toast.svelte Outdated Show resolved Hide resolved
src/app/shared/Note.svelte Show resolved Hide resolved
.husky/pre-commit Outdated Show resolved Hide resolved
src/app/shared/Note.svelte Outdated Show resolved Hide resolved
src/engine/state.ts Outdated Show resolved Hide resolved
@@ -138,7 +133,7 @@
id: note.id,
anonymous: Boolean(note.wrap),
})
.cx({callback: addToContext})
.cx({callback: repository.publish})
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can actually be removed. We might be able to get rid of callback from Zap as well if it's not used anywhere else.

Comment on lines +9 to +10
$: total = thunk.request.relays.length
$: pending = Object.values($status).filter(s => s.status === PublishStatus.Pending).length
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs to remove LOCAL_RELAY_URL from the count

@@ -139,11 +140,28 @@
],
}),
})
const thunk = publish({
event: template,
relays: ctx.app.router.PublishEvent(template).getUrls(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

This template doesn't have a pubkey yet, so random relays are selected. You'll need to sign the event first.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, when using nip 46, there's a pretty long delay before a note gets published. This isn't really part of this PR, but go ahead and add loading to NoteCreate to avoid double publishes.

Comment on lines +58 to +65
<Field>
<div slot="label" class="flex justify-between">
<strong>Send Delay</strong>
<div>{values.send_delay} ms</div>
</div>
<Input type="range" step="500" bind:value={values.send_delay} min={0} max={15_000}></Input>
<p slot="info">A delay period allowing you to cancel a reply or note creation, in seconds.</p>
</Field>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's display seconds instead of ms. A step of 1 is fine

export const publishes = withGetter(writable<Record<string, PublishInfo>>({}))
export const groupHints = withGetter(writable<Record<string, string[]>>({}))
Copy link
Collaborator

Choose a reason for hiding this comment

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

groupHints is old code, it can be removed

Comment on lines -81 to +94
{#key "key"}
{#key $toast.id}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This was actually correct, it's to avoid an animation when there is already a toast visible. Unless something changed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants