Skip to content

Commit

Permalink
perf: reduce number of avatar requests
Browse files Browse the repository at this point in the history
Signed-off-by: Hamza Mahjoubi <hamzamahjoubi221@gmail.com>
hamza221 committed Dec 12, 2024
1 parent 1309b9e commit e76643a
Showing 9 changed files with 54 additions and 94 deletions.
5 changes: 0 additions & 5 deletions appinfo/routes.php
Original file line number Diff line number Diff line change
@@ -275,11 +275,6 @@
'url' => '/api/messages/{messageId}/smartreply',
'verb' => 'GET'
],
[
'name' => 'avatars#url',
'url' => '/api/avatars/url/{email}',
'verb' => 'GET'
],
[
'name' => 'avatars#image',
'url' => '/api/avatars/image/{email}',
37 changes: 0 additions & 37 deletions lib/Controller/AvatarsController.php
Original file line number Diff line number Diff line change
@@ -34,43 +34,6 @@ public function __construct(string $appName,
$this->uid = $UserId;
}

/**
* @NoAdminRequired
* @NoCSRFRequired
*
* @param string $email
* @return JSONResponse
*/
#[TrapError]
public function url(string $email): JSONResponse {
if (empty($email)) {
return new JSONResponse([], Http::STATUS_BAD_REQUEST);
}

if (!filter_var($email, FILTER_VALIDATE_EMAIL)) {
return new JSONResponse([], Http::STATUS_BAD_REQUEST);
}

$avatar = $this->avatarService->getAvatar($email, $this->uid);
if (is_null($avatar)) {
// No avatar found
$response = new JSONResponse([], Http::STATUS_NOT_FOUND);

// Debounce this a bit
// (cache for one day)
$response->cacheFor(24 * 60 * 60, false, true);

return $response;
}

$response = new JSONResponse($avatar);

// Let the browser cache this for a week
$response->cacheFor(7 * 24 * 60 * 60, false, true);

return $response;
}

/**
* @NoAdminRequired
* @NoCSRFRequired
18 changes: 18 additions & 0 deletions lib/Db/Message.php
Original file line number Diff line number Diff line change
@@ -12,6 +12,7 @@
use Horde_Mail_Rfc822_Identification;
use JsonSerializable;
use OCA\Mail\AddressList;
use OCA\Mail\Service\Avatar\Avatar;
use OCP\AppFramework\Db\Entity;
use ReturnTypeWillChange;
use function in_array;
@@ -133,6 +134,8 @@ class Message extends Entity implements JsonSerializable {
/** @var Tag[] */
private $tags = [];

/** @var Avatar|null */
private $avatar;
public function __construct() {
$this->from = new AddressList([]);
$this->to = new AddressList([]);
@@ -283,6 +286,20 @@ public function setFlag(string $flag, bool $value = true) {
);
}
}
/**
* @param Avatar|null $avatar
* @return void
*/
public function setAvatar(?Avatar $avatar): void {
$this->avatar = $avatar;

Check warning on line 294 in lib/Db/Message.php

Codecov / codecov/patch

lib/Db/Message.php#L293-L294

Added lines #L293 - L294 were not covered by tests
}

/**
* @return Avatar
*/
public function getAvatar(): ?Avatar {
return $this->avatar;

Check warning on line 301 in lib/Db/Message.php

Codecov / codecov/patch

lib/Db/Message.php#L300-L301

Added lines #L300 - L301 were not covered by tests
}

#[ReturnTypeWillChange]
public function jsonSerialize() {
@@ -327,6 +344,7 @@ static function (Tag $tag) {
'previewText' => $this->getPreviewText(),
'encrypted' => ($this->isEncrypted() === true),
'mentionsMe' => $this->getMentionsMe(),
'avatar' => $this->avatar ? $this->avatar->jsonSerialize() : null,

Check warning on line 347 in lib/Db/Message.php

Codecov / codecov/patch

lib/Db/Message.php#L347

Added line #L347 was not covered by tests
];
}
}
24 changes: 21 additions & 3 deletions lib/IMAP/PreviewEnhancer.php
Original file line number Diff line number Diff line change
@@ -15,6 +15,7 @@
use OCA\Mail\Db\Message;
use OCA\Mail\Db\MessageMapper as DbMapper;
use OCA\Mail\IMAP\MessageMapper as ImapMapper;
use OCA\Mail\Service\AvatarService;
use Psr\Log\LoggerInterface;
use function array_key_exists;
use function array_map;
@@ -34,14 +35,24 @@ class PreviewEnhancer {
/** @var LoggerInterface */
private $logger;

/** @var AvatarService */
private $avatarService;

/** @var string */
private $UserId;

public function __construct(IMAPClientFactory $clientFactory,
ImapMapper $imapMapper,
DbMapper $dbMapper,
LoggerInterface $logger) {
LoggerInterface $logger,
AvatarService $avatarService,
string $UserId) {
$this->clientFactory = $clientFactory;
$this->imapMapper = $imapMapper;
$this->mapper = $dbMapper;
$this->logger = $logger;
$this->avatarService = $avatarService;
$this->UserId = $UserId;

Check warning on line 55 in lib/IMAP/PreviewEnhancer.php

Codecov / codecov/patch

lib/IMAP/PreviewEnhancer.php#L54-L55

Added lines #L54 - L55 were not covered by tests
}

/**
@@ -50,9 +61,13 @@ public function __construct(IMAPClientFactory $clientFactory,
* @return Message[]
*/
public function process(Account $account, Mailbox $mailbox, array $messages): array {
$needAnalyze = array_reduce($messages, static function (array $carry, Message $message) {
$needAnalyze = array_reduce($messages, function (array $carry, Message $message) {

Check warning on line 64 in lib/IMAP/PreviewEnhancer.php

Codecov / codecov/patch

lib/IMAP/PreviewEnhancer.php#L64

Added line #L64 was not covered by tests
if ($message->getStructureAnalyzed()) {
// Nothing to do
if ($message->getAvatar() === null) {
$avatar = $this->avatarService->getAvatar($message->getFrom()->first()->getEmail(), $this->UserId);

Check failure on line 68 in lib/IMAP/PreviewEnhancer.php

GitHub Actions / static-psalm-analysis dev-master

PossiblyNullArgument

lib/IMAP/PreviewEnhancer.php:68:48: PossiblyNullArgument: Argument 1 of OCA\Mail\Service\AvatarService::getAvatar cannot be null, possibly null value provided (see https://psalm.dev/078)

Check failure on line 68 in lib/IMAP/PreviewEnhancer.php

GitHub Actions / static-psalm-analysis dev-master

PossiblyNullReference

lib/IMAP/PreviewEnhancer.php:68:78: PossiblyNullReference: Cannot call method getEmail on possibly null value (see https://psalm.dev/083)
$message->setAvatar($avatar);

Check warning on line 69 in lib/IMAP/PreviewEnhancer.php

Codecov / codecov/patch

lib/IMAP/PreviewEnhancer.php#L67-L69

Added lines #L67 - L69 were not covered by tests
}
return $carry;
}

@@ -83,7 +98,7 @@ public function process(Account $account, Mailbox $mailbox, array $messages): ar
$client->logout();
}

return $this->mapper->updatePreviewDataBulk(...array_map(static function (Message $message) use ($data) {
return $this->mapper->updatePreviewDataBulk(...array_map(function (Message $message) use ($data) {

Check warning on line 101 in lib/IMAP/PreviewEnhancer.php

Codecov / codecov/patch

lib/IMAP/PreviewEnhancer.php#L101

Added line #L101 was not covered by tests
if (!array_key_exists($message->getUid(), $data)) {
// Nothing to do
return $message;
@@ -97,6 +112,9 @@ public function process(Account $account, Mailbox $mailbox, array $messages): ar
$message->setEncrypted($structureData->isEncrypted());
$message->setMentionsMe($structureData->getMentionsMe());

$avatar = $this->avatarService->getAvatar($message->getFrom()->first()->getEmail(), $this->UserId);

Check failure on line 115 in lib/IMAP/PreviewEnhancer.php

GitHub Actions / static-psalm-analysis dev-master

PossiblyNullArgument

lib/IMAP/PreviewEnhancer.php:115:46: PossiblyNullArgument: Argument 1 of OCA\Mail\Service\AvatarService::getAvatar cannot be null, possibly null value provided (see https://psalm.dev/078)

Check failure on line 115 in lib/IMAP/PreviewEnhancer.php

GitHub Actions / static-psalm-analysis dev-master

PossiblyNullReference

lib/IMAP/PreviewEnhancer.php:115:76: PossiblyNullReference: Cannot call method getEmail on possibly null value (see https://psalm.dev/083)
$message->setAvatar($avatar);

Check warning on line 116 in lib/IMAP/PreviewEnhancer.php

Codecov / codecov/patch

lib/IMAP/PreviewEnhancer.php#L115-L116

Added lines #L115 - L116 were not covered by tests

return $message;
}, $messages));
}
20 changes: 12 additions & 8 deletions src/components/Avatar.vue
Original file line number Diff line number Diff line change
@@ -17,7 +17,7 @@

<script>
import NcAvatar from '@nextcloud/vue/dist/Components/NcAvatar.js'
import { fetchAvatarUrlMemoized } from '../service/AvatarService.js'
import { generateUrl } from '@nextcloud/router'
import logger from '../logger.js'

export default {
@@ -30,6 +30,10 @@ export default {
type: String,
required: true,
},
avatar: {
type: Object,
default: null,
},
email: {
type: String,
required: true,
@@ -55,14 +59,14 @@ export default {
},
},
async mounted() {
if (this.email !== '') {
try {
this.avatarUrl = await fetchAvatarUrlMemoized(this.email)
} catch {
logger.debug('Could not fetch avatar', { email: this.email })
}
if (this.avatar) {
this.avatarUrl = this.avatar.isExternal
? generateUrl('/apps/mail/api/avatars/image/{email}', {
email: this.email,
})
: this.avatar.url
}

logger.debug('Could not fetch avatar', { email: this.email })
this.loading = false
},
}
2 changes: 1 addition & 1 deletion src/components/Envelope.vue
Original file line number Diff line number Diff line change
@@ -52,7 +52,7 @@
<CheckIcon :size="40" class="check-icon" :class="{ 'app-content-list-item-avatar-selected': selected }" />
</template>
<template v-else>
<Avatar :display-name="addresses" :email="avatarEmail" />
<Avatar :display-name="addresses" :email="avatarEmail" :avatar="data.avatar" />
</template>
</div>
</template>
2 changes: 1 addition & 1 deletion src/components/OutboxMessageListItem.vue
Original file line number Diff line number Diff line change
@@ -10,7 +10,7 @@
:details="details"
@click="openModal">
<template #icon>
<Avatar :display-name="avatarDisplayName" :email="avatarEmail" />
<Avatar :display-name="avatarDisplayName" :email="avatarEmail" :avatar="message.avatar" />
</template>
<template #subname>
{{ subjectForSubtitle }}
1 change: 1 addition & 0 deletions src/components/ThreadEnvelope.vue
Original file line number Diff line number Diff line change
@@ -26,6 +26,7 @@
:display-name="envelope.from[0].label"
:disable-tooltip="true"
:size="40"
:avatar="envelope.avatar"
class="envelope__header__avatar-avatar" />
<div v-if="isImportant"
class="app-content-list-item-star icon-important"
39 changes: 0 additions & 39 deletions src/service/AvatarService.js

This file was deleted.

0 comments on commit e76643a

Please sign in to comment.