feat: show recent people on contact icon - #62401
Conversation
7e0645c to
db0b5e6
Compare
| } | ||
| } | ||
| teams.value = [...userTeams] | ||
| await loadPreviewAvatars() |
There was a problem hiding this comment.
Fires twice: setting selectedTeam above already triggers the new watcher. Dropping this call should be enough I think?
| const view = render(ContactsMenu) | ||
| await view.findByRole('button') | ||
|
|
||
| expect(view.container.querySelector('.contactsmenu__trigger-avatars')).toBeNull() |
There was a problem hiding this comment.
Wondering if this runs before loadPreviewAvatars resolves, so it could pass even with the threshold set to >= 1. Perhaps we can add a similar vi.waitFor from the test below?
| margin-inline-end: calc(2 * var(--default-grid-baseline)); | ||
|
|
||
| :deep(.header-menu__trigger) { | ||
| opacity: 1 !important; |
There was a problem hiding this comment.
What is this important working around? To remove the hover/focus feedback?
susnux
added
enhancement
3. to review
db0b5e6 to
def4c5c
Compare
|
@pringelmann Thank you for your comments. |
Signed-off-by: Kristian Zendato <kristian.zendato@nextcloud.com>
def4c5c to
7a3859a
Compare
| return []; | ||
| } | ||
|
|
||
| $entries = $this->manager->getEntries($user, ''); |
There was a problem hiding this comment.
Do I understand correctly that we are now calling this on every single page load? I wonder if retrieving all contacts without a filter like this will impact performance. Would you mind briefly investigating?
There was a problem hiding this comment.
I had a quick look myself: searching with an empty filter takes the recent-statuses path, which does one address book search per recent status, up to 25, then runs every action provider over all 25 results. We keep 3. Those searches can't use an index, so they get slower as the directory grows.
Nothing waits on the response, so it's not a latency problem, but it is recurring database work on every page load.
Limiting the query to 3 instead of 25 and skipping the action providers would cover most of it. Caching per user for a few minutes on top would make repeat loads free, and slightly stale recent contacts are fine.
There was a problem hiding this comment.
Agree, I think even 5 minutes caching would be fine here
| }) | ||
|
|
||
| // immediate: load header avatars on mount and whenever the team filter changes | ||
| watch(selectedTeam, loadPreviewAvatars, { immediate: true }) |
There was a problem hiding this comment.
@kristian-zendato I didn't mean to add a second watcher here. This one has the same problem: it fires immediately, then always fires a second time we set selectedTeam above. Also since we can't predict in which order the responses arrive, this can lead to race conditions.
storage.getItem is synchronous, so perhaps we can seed the ref instead of assigning in onMounted:
const storedTeam = storage.getItem('core:contacts:team')
const selectedTeam = ref<string>(storedTeam ? JSON.parse(storedTeam) : '$_all_$')Then drop the if (team) block. One request, correct team, all cases.
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>
Address remaining review feedback: document why the avatar trigger clears NcHeaderMenu's icon mask, and wait for the icon assertion in the preview-avatars unit test. Rebuild assets after merging master. Assisted-by: Cursor:Composer Co-authored-by: Cursor <cursoragent@cursor.com>
SebastianKrupinski
left a comment
SebastianKrupinski
left a comment
There was a problem hiding this comment.
Please submit this PR without the compiled JS files
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
5 participants
Summary
Makes the People (contacts) header menu more inviting by showing a stack of up to 3 overlapping avatars of recent contacts on the menu trigger, instead of only the contacts icon.
GET /contactsmenu/preview-avatarswhich returns the first 3 entries from the same unfiltered contacts list as the menu (Manager::getEntrieswith an empty filter), with optional team filtering matching the existing index endpoint.NcAvatarstack when at least 2 are available; otherwise keeps the existing contacts icon.width: fit-content) so the stack does not overlap notifications/profile and remains fully clickable.Checklist
3. to review, feature component)stable32)AI (if applicable)