Skip to content

feat: split envelopes by time of arrival #11264

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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Conversation

GretaD
Copy link
Contributor

@GretaD GretaD commented Jun 12, 2025

fixes #11057

to do

  • do not show an empty section

@GretaD GretaD self-assigned this Jun 12, 2025
@ChristophWurst
Copy link
Member

For reactivity: would it make sense to maintain the brackets in Pinia? Brackets relative to current time are probably not reactive. But if we commit the timestamp to Pinia, e.g. at every synchronization, and re-evaluate, it should be fine.

Copy link
Member

@st3iny st3iny left a comment

Choose a reason for hiding this comment

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

There is a problem with an empty view initially. When the app loads there are no envelopes at first. Fetching initial envelopes is handled inside Mailbox.vue which is not rendered as there are no envelopes initially (and no groups as a consequence). As a result, no envelopes will be fetched and rendered until the first background sync hits.

My suggestions:

  1. Move the sections/groups from MailboxThread.vue into Mailbox.vue.
  2. Move some of the envelope fetching logic from Mailbox.vue to MailboxThread.vue (have a look at mounted(), loadEnvelopes() and prefetchOtherMailboxes() in Mailbox.vue).

In any case, you should rebase against latest origin/main soon as I implemented a lot of changes to the sync logic in Mailbox.vue.

@GretaD GretaD force-pushed the feat/add-byDate-sections branch from 6bb373f to d95f003 Compare July 1, 2025 12:33
@ChristophWurst
Copy link
Member

For reactivity: would it make sense to maintain the brackets in Pinia? Brackets relative to current time are probably not reactive. But if we commit the timestamp to Pinia, e.g. at every synchronization, and re-evaluate, it should be fine.

Bump. You have a computed property that depends on time. How does Vue handle this side effect? Is the window for messages in the last hour evaluated again as time passes?

@GretaD
Copy link
Contributor Author

GretaD commented Jul 8, 2025

For reactivity: would it make sense to maintain the brackets in Pinia? Brackets relative to current time are probably not reactive. But if we commit the timestamp to Pinia, e.g. at every synchronization, and re-evaluate, it should be fine.

Bump. You have a computed property that depends on time. How does Vue handle this side effect? Is the window for messages in the last hour evaluated again as time passes?

you are right, the current one is not the ideal solution, i will do what you suggested and commit the timestamp to Pinia. I was focused on fixing the bugs i caused with this feature and didnt pay attention to this part yet.

@GretaD GretaD force-pushed the feat/add-byDate-sections branch from 1ce13ea to 4b56871 Compare July 24, 2025 09:32
@GretaD GretaD marked this pull request as ready for review July 24, 2025 11:48
@@ -161,6 +192,7 @@ export default {
await this.prefetchOtherMailboxes()

this.mainStore.setHasFetchedInitialEnvelopesMutation(true)
this.mainStore.updateSyncTimestamp()
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 only occasion the timestamp is updated. This means the view is not reacting and regrouping when you have the app open for a while

How about updating the time stamp at every sync?

@@ -91,6 +112,10 @@ export default {
required: false,
default: false,
},
customEnvelopes: {
Copy link
Member

Choose a reason for hiding this comment

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

This seems mostly unused? It's read once, but never assigned

:load-more-button="false"
:skip-transition="skipListTransition"
@delete="onDelete"
@load-more="loadMore" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't forget about the load more button ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we removed them on purpose, if thats what you mean

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, i see now :)

startOfLastMonth.setMonth(startOfLastMonth.getMonth() - 1)

const groups = {
[t('mail', 'Last hour')]: [],
Copy link
Contributor

Choose a reason for hiding this comment

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

I will suggest a different approach later because I find it a bit weird to use a translate string as key.

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

Successfully merging this pull request may close these issues.

Thread list separators depending on sort attribute
4 participants