Conversation
tammi-23
added
Type:Feature-Request
Type:Enhancement
and removed
Type:Feature-Request
labels
There was a problem hiding this comment.
Pull request overview
Implements draft state handling for the mail compose flow, including save/discard behaviors and draft mailbox discovery.
Changes:
- Added a draft API connector + composable to create/replace/delete drafts and to manage dirty/saving state.
- Refactored Pinia stores to sync selected IDs with route query parameters via a shared helper.
- Updated UI to prompt on close, auto-save drafts periodically, and show a “Saved” hint.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/web-app-mail/src/helpers/mailDraftConnector.ts | Adds an HTTP-backed connector implementing the draft API. |
| packages/web-app-mail/src/composables/useSaveAsDraft.ts | Introduces draft-saving/discarding composable with dirty/saving state. |
| packages/web-app-mail/src/composables/piniaStores/mails.ts | Refactors selected mail handling to use route query binding helper. |
| packages/web-app-mail/src/composables/piniaStores/mailboxes.ts | Refactors mailboxes store and adds computed drafts mailbox selection. |
| packages/web-app-mail/src/composables/piniaStores/accounts.ts | Refactors accounts store and binds current account to route query. |
| packages/web-app-mail/src/composables/piniaStores/helpers.ts | Adds helper to normalize route query values into a single string. |
| packages/web-app-mail/src/components/MailboxTree.vue | Fixes mailbox selection flow to avoid deref’ing missing account. |
| packages/web-app-mail/src/components/MailWidget.vue | Adds leave-confirm modal, auto-save, and draft integration to compose modal. |
| packages/web-app-mail/src/components/MailList.vue | Mounts compose widget only when open; fixes seen-flag update source. |
| packages/web-app-mail/src/components/MailListItem.vue | Sanitizes HTML-like preview content before rendering preview text. |
| packages/web-app-mail/src/components/MailComposeForm.vue | Stabilizes props defaults + fixes modelValue usage after toRefs(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const accountId = unref(currentAccount)?.accountId | ||
| if (!accountId) { | ||
| return | ||
| } |
There was a problem hiding this comment.
We don't need to check this here, if we are selecting a mailbox and no account is set, this is a bug, which might break the whole app. We should rather check, when this is happening and fix the root cause
| modelValue: ComposeFormState | ||
| showFormattingToolbar?: boolean | ||
| }>() | ||
| const props = withDefaults( |
There was a problem hiding this comment.
Since you only set showFormattingToolbar to false and not set a complex object, I don't think this is necessary, if not in the consuming component, it should be false either way
| } | ||
| ) | ||
|
|
||
| const { modelValue, showFormattingToolbar } = toRefs(props) |
There was a problem hiding this comment.
Usually, we don't do this, please check if you can find away around
|
|
||
| const updateField = <K extends keyof ComposeFormState>(key: K, value: ComposeFormState[K]) => { | ||
| emit('update:modelValue', { ...modelValue, [key]: value }) | ||
| emit('update:modelValue', { ...modelValue.value, [key]: value }) |
There was a problem hiding this comment.
please use unref, only use .value if you want to set the prop
| </li> | ||
| </oc-list> | ||
| <MailWidget v-model="showCompose" /> | ||
| <MailWidget v-if="showCompose" v-model="showCompose" /> |
There was a problem hiding this comment.
I think we can kick v-model now
| }) | ||
|
|
||
| const previewText = computed(() => { | ||
| const p = mail.preview ?? '' |
There was a problem hiding this comment.
Just check if empty and do eraly return if so, otherwise just jagg it trough the sanitizer
| <MailComposeAttachmentButton | ||
| v-model="composeState.attachments" | ||
| :account-id="composeState.from?.accountId" | ||
| :account-id="composeState.from?.accountId ?? accountId" |
There was a problem hiding this comment.
Please use currentAccount only, and make sure that we only show sender emails from the currentAccount, as for now, sending mails from different accounts seems to unstable to me
| <MailComposeAttachmentButton | ||
| v-model="composeState.attachments" | ||
| :account-id="composeState.from?.accountId" | ||
| :account-id="composeState.from?.accountId ?? accountId" |
| <oc-icon name="text" fill-type="none" class="text-base text-role-on-surface" /> | ||
| </oc-button> | ||
| <div class="ml-auto flex items-center min-w-0"> | ||
| <div |
There was a problem hiding this comment.
Maybe we can make use of a reusable component
| </div> | ||
| </template> | ||
| </oc-modal> | ||
| <oc-modal v-if="leaveModalOpen" :title="$gettext('Leave this screen?')" :hide-actions="true"> |
There was a problem hiding this comment.
Please make a new component out of it
|
|
||
| const { showSavedHint, flashSavedHint, clearSavedHint } = useSavedHint(SAVED_HINT_DURATION_MS) | ||
|
|
||
| const accountId = computed(() => unref(currentAccount)?.accountId ?? '') |
| const draftMailboxId = computed(() => unref(draftsMailboxId) ?? '') | ||
|
|
||
| const canSaveDraft = computed(() => { | ||
| return !!accountId.value && !!draftMailboxId.value |
There was a problem hiding this comment.
accountId should always be set I don't think we need to check, please use unref
| currentAccountIdQuery.value = data?.accountId | ||
| } | ||
| const hasAccounts = accounts.value.length > 0 | ||
| const hasValidCurrent = !!currentAccount.value |
There was a problem hiding this comment.
We don't want to set the currentAccount if we fill the accounts list. If you came across a bug and tried to fix it here. But this is not the right place,
if the bug reoccures please check if there is an issue in the initial loader / setter
in packages/web-app-mail/src/views/Inbox.vue
| @@ -0,0 +1,20 @@ | |||
| import { computed, type Ref } from 'vue' | |||
| if (mailbox) { | ||
| mailbox[field] = value | ||
| if (!currentMailboxId.value && mailboxes.value.length) { | ||
| currentMailboxId.value = mailboxes.value[0].id |
There was a problem hiding this comment.
We don't want to set the currentMailbox here, if we just fill the list, I think you came accross a bug and tried to fix it here. But this is not the right place,
if the bug reoccures please check if there is an issue in the initial loader / setter
in packages/web-app-mail/src/views/Inbox.vue
| currentMailId.value = null | ||
| currentMailIdQuery.value = null | ||
| if (currentMailId.value && !currentMail.value) { | ||
| currentMailId.value = '' |
There was a problem hiding this comment.
We don't want to set the currentMail if we just fill the mails list, I think you came accross a bug and tried to fix it here. But this is not the right place,
if the bug reoccures please check if there is an issue in the initial loader / setter
in packages/web-app-mail/src/views/Inbox.vue
| return (value ?? '').replace(/\s+/g, ' ').trim() | ||
| } | ||
|
|
||
| export const plainTextForChangeCheck = (html: string) => { |
There was a problem hiding this comment.
you don't need this, just use v-html="props.text" in your component
| } | ||
|
|
||
| if (!raw.includes('<')) { | ||
| return normalizeWhitespace(raw) |
There was a problem hiding this comment.
We shouldn't replace the user input; If the white spaces bother us in the preview, we should adjust the representation
| return segments.map((segment) => encodeURIComponent(segment)).join('/') | ||
| } | ||
|
|
||
| export function createMailDraftConnector(http: HttpLike, groupwareUrl: string): MailDraftApi { |
There was a problem hiding this comment.
- Please make a real compososable out of that
- We don't need to pass the clientService and configStore props, just use e.G useClientService
- Kill the HttpLike interface, it's useless as clientService has it's own interface
2 participants
implement #1480