Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 44 additions & 0 deletions core/Controller/ContactsMenuController.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,21 @@
use OCP\AppFramework\Http\Attribute\NoAdminRequired;
use OCP\AppFramework\Http\JSONResponse;
use OCP\Contacts\ContactsMenu\IEntry;
use OCP\ICacheFactory;
use OCP\IRequest;
use OCP\IUserSession;
use OCP\Teams\ITeamManager;

class ContactsMenuController extends Controller {
private const PREVIEW_AVATARS_LIMIT = 3;
private const PREVIEW_AVATARS_CACHE_TTL = 300;

public function __construct(
IRequest $request,
private IUserSession $userSession,
private Manager $manager,
private ITeamManager $teamManager,
private ICacheFactory $cacheFactory,
) {
parent::__construct('core', $request);
}
Expand Down Expand Up @@ -70,4 +75,43 @@ public function findOne(int $shareType, string $shareWith) {
public function getTeams(): array {
return $this->teamManager->getTeamsForUser($this->userSession->getUser()->getUID());
}

/**
* Top contacts for the People menu header avatar stack (max 3).
* Uses a lightweight query (limited results, no action providers) and
* caches per user for a few minutes.
*
* @return list<array>
* @throws Exception
*/
#[NoAdminRequired]
#[FrontpageRoute(verb: 'GET', url: '/contactsmenu/preview-avatars')]
public function previewAvatars(?string $teamId = null): array {
Comment on lines +87 to +89

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please add rate limiting

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

UserRateLimit(limit: 60, period: 120) would be enough for it?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Tbh, I don't know what a reasonable threshold might be... I can't see a regular user reloading the page every 2s?

Make the span 5min, and the limit 30 times... even that is over kill i think

$user = $this->userSession->getUser();
if ($user === null) {
return [];
}

$cache = $this->cacheFactory->createDistributed('contactsmenu-preview');
$cacheKey = $user->getUID();
$cached = $cache->get($cacheKey);
if (!is_array($cached)) {
Comment on lines +95 to +98

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am not sure if the caching is worth it here. The cache TTL is 5min, which means most likely the cache will never get utilized that much.

Most users will not flip between the apps that frequently, they will click on mail work there for 10min then maybe change over the calendar extra. Power users will just open each app in a separate window, so not sure this will really help any.

@kesselb @DerDreschner what do you think?

$entries = $this->manager->getPreviewEntries($user, self::PREVIEW_AVATARS_LIMIT);
$cached = array_map(
static fn (IEntry $entry): array => $entry->jsonSerialize(),
$entries,
);
$cache->set($cacheKey, $cached, self::PREVIEW_AVATARS_CACHE_TTL);
}

if ($teamId !== null && $teamId !== '') {
$memberIds = $this->teamManager->getMembersOfTeam($teamId, $user->getUID());
$cached = array_filter(
$cached,
static fn (array $entry): bool => array_key_exists($entry['uid'] ?? '', $memberIds)
);
}

return array_values(array_slice($cached, 0, self::PREVIEW_AVATARS_LIMIT));
}
}
51 changes: 51 additions & 0 deletions core/src/tests/views/ContactsMenu.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,20 @@ vi.mock('@nextcloud/auth', () => ({

afterEach(cleanup)

function mockDefaultGets(previewUsers: Array<{ uid: string, fullName: string, isUser?: boolean }> = []) {
axios.get.mockImplementation(async (url: string) => {
if (String(url).includes('/contactsmenu/preview-avatars')) {
return {
data: previewUsers.map((user) => ({
isUser: true,
...user,
})),
}
}
return { data: [] }
})
}

describe('ContactsMenu', function() {
it('shows a loading text', async () => {
const { promise, resolve } = Promise.withResolvers<void>()
Expand Down Expand Up @@ -124,4 +138,41 @@ describe('ContactsMenu', function() {
expect(items[0]!.textContent).toContain('Acosta Lancaster')
expect(items[1]!.textContent).toContain('Adeline Snider')
})

it('shows the contacts icon when fewer than two preview users are available', async () => {
mockDefaultGets([{ uid: 'alice', fullName: 'Alice', isUser: true }])
axios.post.mockResolvedValue({
data: { contacts: [], contactsAppEnabled: false },
})

const view = render(ContactsMenu)
await view.findByRole('button')

await vi.waitFor(() => {
expect(axios.get.mock.calls.some(([url]) => String(url).includes('/contactsmenu/preview-avatars'))).toBe(true)
expect(view.container.querySelector('.contactsmenu__trigger-avatars')).toBeNull()
expect(view.container.querySelector('.contactsmenu__trigger-icon')).toBeTruthy()
})
})

it('shows an avatar stack when at least two preview users are available', async () => {
mockDefaultGets([
{ uid: 'alice', fullName: 'Alice', isUser: true },
{ uid: 'contact-1', fullName: 'External Contact', isUser: false },
{ uid: 'bob', fullName: 'Bob', isUser: true },
])
axios.post.mockResolvedValue({
data: { contacts: [], contactsAppEnabled: false },
})

const view = render(ContactsMenu)
await view.findByRole('button')

// wait for onMounted preview load
await vi.waitFor(() => {
expect(view.container.querySelector('.contactsmenu__trigger-avatars')).toBeTruthy()
})
expect(view.container.querySelectorAll('.contactsmenu__trigger-avatars__avatar')).toHaveLength(3)
expect(view.container.querySelector('.contactsmenu__trigger-icon')).toBeNull()
})
})
118 changes: 110 additions & 8 deletions core/src/views/ContactsMenu.vue
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import debounce from 'debounce'
import { computed, nextTick, onMounted, ref, watch } from 'vue'
import NcActionButton from '@nextcloud/vue/components/NcActionButton'
import NcActions from '@nextcloud/vue/components/NcActions'
import NcAvatar from '@nextcloud/vue/components/NcAvatar'
import NcButton from '@nextcloud/vue/components/NcButton'
import NcEmptyContent from '@nextcloud/vue/components/NcEmptyContent'
import NcHeaderMenu from '@nextcloud/vue/components/NcHeaderMenu'
Expand All @@ -23,6 +24,12 @@ import NcTextField from '@nextcloud/vue/components/NcTextField'
import ContactMenuEntry from '../components/ContactsMenu/ContactMenuEntry.vue'
import logger from '../logger.js'

interface IPreviewUser {
uid: string
fullName: string
isUser: boolean
}

Comment on lines +27 to +32

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Interfaces should be extracted in to a separate types file "src/types"

const storage = getBuilder('core:contacts')
.persist(true)
.clearOnLogout(true)
Expand All @@ -42,15 +49,13 @@ const hasError = ref(false)
const searchTerm = ref('')

const teams = ref<ITeam[]>([])
const selectedTeam = ref<string>('$_all_$')
const storedTeam = storage.getItem('core:contacts:team')
const selectedTeam = ref<string>(storedTeam ? JSON.parse(storedTeam) : '$_all_$')
const selectedTeamName = computed(() => teams.value.find((t) => t.teamId === selectedTeam.value)?.displayName)
const previewUsers = ref<IPreviewUser[]>([])
const showAvatarStack = computed(() => previewUsers.value.length >= 2)

onMounted(async () => {
const team = storage.getItem('core:contacts:team')
if (team) {
selectedTeam.value = JSON.parse(team)
}

if (userTeams.length === 0) {
try {
const { data } = await axios.get<ITeam[]>(generateUrl('/contactsmenu/teams'))
Expand All @@ -65,8 +70,29 @@ onMounted(async () => {
watch(selectedTeam, () => {
storage.setItem('core:contacts:team', JSON.stringify(selectedTeam.value))
getContacts(searchTerm.value)
loadPreviewAvatars()
})

/**
* Load avatars for the People menu header trigger
*/
async function loadPreviewAvatars() {
try {
const { data } = await axios.get<IPreviewUser[]>(generateUrl('/contactsmenu/preview-avatars'), {
params: {
teamId: selectedTeam.value !== '$_all_$' ? selectedTeam.value : undefined,
},
})
previewUsers.value = data
} catch (error) {
logger.error('could not load preview avatars', { error })
previewUsers.value = []
}
}

// Seeded selectedTeam above so this runs once on mount with the correct team
loadPreviewAvatars()

/**
* Load contacts when opening the menu
*/
Expand Down Expand Up @@ -145,11 +171,32 @@ const userTeams: ITeam[] = []
<NcHeaderMenu
id="contactsmenu"
class="contactsmenu"
:class="{ 'contactsmenu--avatar-stack': showAvatarStack }"
:aria-label="t('core', 'Search contacts')"
exclude-click-outside-selectors=".v-popper__popper"
@open="onOpened">
<template #trigger>
<NcIconSvgWrapper class="contactsmenu__trigger-icon" :path="mdiContacts" />
<span
v-if="showAvatarStack"
class="contactsmenu__trigger-avatars"
aria-hidden="true">
<NcAvatar
v-for="(previewUser, index) in previewUsers"
:key="previewUser.isUser ? previewUser.uid : `${previewUser.fullName}-${index}`"
class="contactsmenu__trigger-avatars__avatar"
:style="{ zIndex: previewUsers.length - index }"
:user="previewUser.isUser ? previewUser.uid : undefined"
:is-no-user="!previewUser.isUser"
:display-name="previewUser.fullName"
:size="32"
disable-menu
disable-tooltip
hide-status />
</span>
<NcIconSvgWrapper
v-else
class="contactsmenu__trigger-icon"
:path="mdiContacts" />
</template>
<div class="contactsmenu__menu">
<div class="contactsmenu__menu__search-container">
Expand Down Expand Up @@ -242,12 +289,67 @@ const userTeams: ITeam[] = []

<style lang="scss" scoped>
.contactsmenu {
overflow-y: hidden;
margin-inline-end: calc(2 * var(--default-grid-baseline));

:deep(.header-menu__trigger) {
// NcHeaderMenu applies --header-menu-icon-mask (vertical alpha fade) to
// .button-vue__icon:not(:has(svg)). Avatars need the full face visible.
.button-vue__icon:has(.contactsmenu__trigger-avatars) {
mask: none !important;
}
}

&--avatar-stack {
width: fit-content !important;
min-width: var(--header-height);
overflow: visible;
flex-shrink: 0;

:deep(.header-menu__trigger) {
width: fit-content !important;
min-width: var(--header-height);
max-width: none;
overflow: visible !important;
padding-inline: var(--default-grid-baseline);

.button-vue__wrapper {
width: auto;
justify-content: center;
}

.button-vue__icon {
width: auto !important;
min-width: 0;
max-width: none;
height: auto;
min-height: 0;
overflow: visible;
}
}
}

&__trigger-icon {
color: var(--color-background-plain-text) !important;
}

&__trigger-avatars {
display: flex;
align-items: center;
pointer-events: none;

&__avatar {
box-sizing: content-box;
flex-shrink: 0;
--contactsmenu-avatar-outline: var(--border-width-input) solid color-mix(in srgb, var(--color-background-plain-text), transparent 75%);
outline: var(--contactsmenu-avatar-outline);
margin-inline-start: -12px;

&:first-child {
margin-inline-start: 0;
}
}
}

&__menu {
display: flex;
flex-direction: column;
Expand Down
17 changes: 17 additions & 0 deletions lib/private/Contacts/ContactsMenu/Manager.php
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,23 @@ public function getEntries(IUser $user, ?string $filter): array {
];
}

/**
* Lightweight recent contacts for the People menu header avatar stack.
* Limits the store query and skips action providers.
*
* @return IEntry[]
* @throws Exception
*/
public function getPreviewEntries(IUser $user, int $limit = 3): array {
$limit = max(0, $limit);
if ($limit === 0) {
return [];
}

$entries = $this->store->getContacts($user, '', $limit);
return array_slice($this->sortEntries($entries), 0, $limit);
Comment on lines +63 to +64

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is the idea to get any 3 contacts? Or just the last 3 recent contacts?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It is to get last 3 recent contacts.

}

/**
* @throws Exception
*/
Expand Down
Loading
Loading