fix(contactsmenu): address preview avatars review feedback

Seed the team filter from storage to avoid a double-fetch race, and load
header avatars via a limited query that skips action providers and caches
per user for 5 minutes.

Assisted-by: Cursor:Composer
Co-authored-by: Cursor <cursoragent@cursor.com>

Author: kristian-zendato

core/Controller/ContactsMenuController.php

@@ -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);
 	}
@@ -73,9 +78,10 @@ public function getTeams(): array {
 
 	/**
 	 * Top contacts for the People menu header avatar stack (max 3).
-	 * Same source/order as the contacts menu with an empty filter.
+	 * Uses a lightweight query (limited results, no action providers) and
+	 * caches per user for a few minutes.
 	 *
-	 * @return list<IEntry>
+	 * @return list<array>
 	 * @throws Exception
 	 */
 	#[NoAdminRequired]
@@ -86,15 +92,26 @@ public function previewAvatars(?string $teamId = null): array {
 			return [];
 		}
 
-		$entries = $this->manager->getEntries($user, '');
+		$cache = $this->cacheFactory->createDistributed('contactsmenu-preview');
+		$cacheKey = $user->getUID();
+		$cached = $cache->get($cacheKey);
+		if (!is_array($cached)) {
+			$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());
-			$entries['contacts'] = array_filter(
-				$entries['contacts'],
-				fn (IEntry $entry) => array_key_exists($entry->getProperty('UID'), $memberIds)
+			$cached = array_filter(
+				$cached,
+				static fn (array $entry): bool => array_key_exists($entry['uid'] ?? '', $memberIds)
 			);
 		}
 
-		return array_values(array_slice($entries['contacts'], 0, 3));
+		return array_values(array_slice($cached, 0, self::PREVIEW_AVATARS_LIMIT));
 	}
 }

core/src/tests/views/ContactsMenu.spec.ts

@@ -150,10 +150,9 @@ describe('ContactsMenu', function() {
 
 		await vi.waitFor(() => {
 			expect(axios.get.mock.calls.some(([url]) => String(url).includes('/contactsmenu/preview-avatars'))).toBe(true)
+			expect(view.container.querySelector('.contactsmenu__trigger-icon')).toBeTruthy()
 		})
-		await new Promise((resolve) => setTimeout(resolve, 0))
 		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 () => {

core/src/views/ContactsMenu.vue

@@ -49,17 +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'))
@@ -74,11 +70,9 @@ onMounted(async () => {
 watch(selectedTeam, () => {
 	storage.setItem('core:contacts:team', JSON.stringify(selectedTeam.value))
 	getContacts(searchTerm.value)
+	loadPreviewAvatars()
 })
 
-// immediate: load header avatars on mount and whenever the team filter changes
-watch(selectedTeam, loadPreviewAvatars, { immediate: true })
-
 /**
  * Load avatars for the People menu header trigger
  */
@@ -96,6 +90,9 @@ async function loadPreviewAvatars() {
 	}
 }
 
+// Seeded selectedTeam above so this runs once on mount with the correct team
+loadPreviewAvatars()
+
 /**
  * Load contacts when opening the menu
  */

lib/private/Contacts/ContactsMenu/Manager.php

@@ -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);
+	}
+
 	/**
 	 * @throws Exception
 	 */

tests/Core/Controller/ContactsMenuControllerTest.php

@@ -10,6 +10,8 @@
 use OC\Contacts\ContactsMenu\Manager;
 use OC\Core\Controller\ContactsMenuController;
 use OCP\Contacts\ContactsMenu\IEntry;
+use OCP\ICache;
+use OCP\ICacheFactory;
 use OCP\IRequest;
 use OCP\IUser;
 use OCP\IUserSession;
@@ -21,6 +23,8 @@ class ContactsMenuControllerTest extends TestCase {
 	private IUserSession&MockObject $userSession;
 	private Manager&MockObject $contactsManager;
 	private ITeamManager&MockObject $teamManager;
+	private ICacheFactory&MockObject $cacheFactory;
+	private ICache&MockObject $cache;
 
 	private ContactsMenuController $controller;
 
@@ -32,12 +36,19 @@ protected function setUp(): void {
 		$this->userSession = $this->createMock(IUserSession::class);
 		$this->contactsManager = $this->createMock(Manager::class);
 		$this->teamManager = $this->createMock(ITeamManager::class);
+		$this->cacheFactory = $this->createMock(ICacheFactory::class);
+		$this->cache = $this->createMock(ICache::class);
+
+		$this->cacheFactory->method('createDistributed')
+			->with('contactsmenu-preview')
+			->willReturn($this->cache);
 
 		$this->controller = new ContactsMenuController(
 			$request,
 			$this->userSession,
 			$this->contactsManager,
 			$this->teamManager,
+			$this->cacheFactory,
 		);
 	}
 
@@ -128,56 +139,82 @@ public function testFindOne404(): void {
 
 	public function testPreviewAvatarsWithoutTeam(): void {
 		$user = $this->createMock(IUser::class);
+		$user->method('getUID')->willReturn('current-user');
+
 		$contacts = [
-			$this->createMock(IEntry::class),
-			$this->createMock(IEntry::class),
-			$this->createMock(IEntry::class),
-			$this->createMock(IEntry::class),
+			$this->createPreviewEntry('alice', 'Alice'),
+			$this->createPreviewEntry('bob', 'Bob'),
+			$this->createPreviewEntry('carol', 'Carol'),
+			$this->createPreviewEntry('dave', 'Dave'),
+		];
+		$expected = [
+			$contacts[0]->jsonSerialize(),
+			$contacts[1]->jsonSerialize(),
+			$contacts[2]->jsonSerialize(),
 		];
 
 		$this->userSession->expects($this->once())
 			->method('getUser')
 			->willReturn($user);
+		$this->cache->expects($this->once())
+			->method('get')
+			->with('current-user')
+			->willReturn(null);
 		$this->contactsManager->expects($this->once())
-			->method('getEntries')
-			->with($user, '')
-			->willReturn([
-				'contacts' => $contacts,
-				'contactsAppEnabled' => true,
-			]);
+			->method('getPreviewEntries')
+			->with($user, 3)
+			->willReturn([$contacts[0], $contacts[1], $contacts[2]]);
+		$this->cache->expects($this->once())
+			->method('set')
+			->with('current-user', $expected, 300);
 		$this->teamManager->expects($this->never())
 			->method('getMembersOfTeam');
 
-		$this->assertEquals([
-			$contacts[0],
-			$contacts[1],
-			$contacts[2],
-		], $this->controller->previewAvatars());
+		$this->assertEquals($expected, $this->controller->previewAvatars());
+	}
+
+	public function testPreviewAvatarsUsesCache(): void {
+		$user = $this->createMock(IUser::class);
+		$user->method('getUID')->willReturn('current-user');
+		$cached = [
+			['uid' => 'alice', 'fullName' => 'Alice', 'isUser' => true],
+			['uid' => 'bob', 'fullName' => 'Bob', 'isUser' => true],
+		];
+
+		$this->userSession->expects($this->once())
+			->method('getUser')
+			->willReturn($user);
+		$this->cache->expects($this->once())
+			->method('get')
+			->with('current-user')
+			->willReturn($cached);
+		$this->contactsManager->expects($this->never())
+			->method('getPreviewEntries');
+		$this->cache->expects($this->never())
+			->method('set');
+
+		$this->assertEquals($cached, $this->controller->previewAvatars());
 	}
 
 	public function testPreviewAvatarsWithTeam(): void {
 		$user = $this->createMock(IUser::class);
 		$user->method('getUID')->willReturn('current-user');
 
-		$alice = $this->createMock(IEntry::class);
-		$alice->method('getProperty')->with('UID')->willReturn('alice');
-		$external = $this->createMock(IEntry::class);
-		$external->method('getProperty')->with('UID')->willReturn('contact-1');
-		$bob = $this->createMock(IEntry::class);
-		$bob->method('getProperty')->with('UID')->willReturn('bob');
-		$carol = $this->createMock(IEntry::class);
-		$carol->method('getProperty')->with('UID')->willReturn('carol');
+		$cached = [
+			['uid' => 'alice', 'fullName' => 'Alice', 'isUser' => true],
+			['uid' => 'contact-1', 'fullName' => 'External', 'isUser' => false],
+			['uid' => 'bob', 'fullName' => 'Bob', 'isUser' => true],
+		];
 
 		$this->userSession->expects($this->once())
 			->method('getUser')
 			->willReturn($user);
-		$this->contactsManager->expects($this->once())
-			->method('getEntries')
-			->with($user, '')
-			->willReturn([
-				'contacts' => [$alice, $external, $bob, $carol],
-				'contactsAppEnabled' => true,
-			]);
+		$this->cache->expects($this->once())
+			->method('get')
+			->with('current-user')
+			->willReturn($cached);
+		$this->contactsManager->expects($this->never())
+			->method('getPreviewEntries');
 		$this->teamManager->expects($this->once())
 			->method('getMembersOfTeam')
 			->with('team-id', 'current-user')
@@ -187,16 +224,31 @@ public function testPreviewAvatarsWithTeam(): void {
 				'carol' => 'Carol',
 			]);
 
-		$this->assertEquals([$alice, $bob, $carol], $this->controller->previewAvatars('team-id'));
+		$this->assertEquals([
+			$cached[0],
+			$cached[2],
+		], $this->controller->previewAvatars('team-id'));
 	}
 
 	public function testPreviewAvatarsWithoutUser(): void {
 		$this->userSession->expects($this->once())
 			->method('getUser')
 			->willReturn(null);
 		$this->contactsManager->expects($this->never())
-			->method('getEntries');
+			->method('getPreviewEntries');
+		$this->cache->expects($this->never())
+			->method('get');
 
 		$this->assertEquals([], $this->controller->previewAvatars());
 	}
+
+	private function createPreviewEntry(string $uid, string $fullName): IEntry&MockObject {
+		$entry = $this->createMock(IEntry::class);
+		$entry->method('jsonSerialize')->willReturn([
+			'uid' => $uid,
+			'fullName' => $fullName,
+			'isUser' => true,
+		]);
+		return $entry;
+	}
 }

tests/lib/Contacts/ContactsMenu/ManagerTest.php

@@ -155,6 +155,27 @@ public function testGetFilteredEntriesMinSearchStringLength(): void {
 		$this->assertEquals($expected, $data);
 	}
 
+	public function testGetPreviewEntries(): void {
+		$user = $this->createMock(IUser::class);
+		$entries = $this->generateTestEntries();
+
+		$this->contactsStore->expects($this->once())
+			->method('getContacts')
+			->with($user, '', 3)
+			->willReturn($entries);
+		$this->actionProviderStore->expects($this->never())
+			->method('getProviders');
+		$this->config->expects($this->never())
+			->method('getSystemValueInt');
+
+		$data = $this->manager->getPreviewEntries($user, 3);
+
+		$this->assertCount(3, $data);
+		$this->assertSame('Contact A', $data[0]->getFullName());
+		$this->assertSame('Contact B', $data[1]->getFullName());
+		$this->assertSame('Contact C', $data[2]->getFullName());
+	}
+
 	public function testFindOne(): void {
 		$shareTypeFilter = 42;
 		$shareWithFilter = 'foobar';