feat: add Office overview page with recent / shared / templates#5598
feat: add Office overview page with recent / shared / templates#5598karlitschek wants to merge 1 commit intomainfrom
Conversation
Adds a new top-level "Office" entry to the app menu that opens a Vue SPA at /apps/richdocuments/overview, providing a single landing page for the user's office work. Features: - Home with three sections: My recent docs, Shared with me, Templates - Dedicated views for each, with date grouping (Today / Yesterday / Earlier this week / Earlier this month / Older) and sticky headers - Type filter pills (Documents / Spreadsheets / Presentations / PDFs) - List and grid view toggle, persisted in localStorage - Hover quick-preview popover (mounted at <body> via singleton) - Active-editor badges with pulsing live indicator (WOPI tokens) - Pinning / favourites integrated with Nextcloud's per-user tags so pins also appear in the Files app's Favorites view - Type-coloured thumbnails with frame on grid cards - Friendly empty-state illustrations with calls to action - Confetti + showSuccess toast on document creation - Smooth fade/slide route transitions, all motion respects prefers-reduced-motion Backend: - OverviewService runs an indexed user-folder SearchQuery (mime IN + 60-day mtime window), partitions by ownership for recent vs shared, and batch-loads active editors and favourites per page - OverviewController renders the SPA shell as RENDER_AS_USER - OverviewApiController exposes paginated OCS endpoints, plus create-from-template and favourite-toggle, with strict input validation and OCS-bypass CSRF handling Frontend: - Vue 2 + vue-router 3 SPA mounted at #content (Files-app pattern) - Reuses Nextcloud's design system primitives (NcContent, NcAppNavigation, NcAvatar, NcDateTime, NcDialog, NcEmptyContent, NcLoadingIcon, NcButton, NcTextField) - Optimistic UI for pin toggling Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Frank Karlitschek <karlitschek@users.noreply.github.com>
There was a problem hiding this comment.
General feedback, as going through the code and picking out everything wrong proved to be too much work. That said, there are still some specific comments I made when looking through the code.
- Navigating away from the "Home" tab and then back again breaks the navigation (/overview works but not /overview/)
- Opening a document from the "My recent documents" section of the "Home" page breaks the URL
- When you try to close the document, you are left at a blank screen unless you manually change the URL
- The "... is editing this document" is not totally accurate (says I am editing but have not been for several minutes)
- Shouldn't you be able to simply make a new document instead of being forced to select a template?
- With the above point in mind, it does not even detect any of my templates so I couldn't create a document in the first place
- css/overview.scss is not used for anything
- There is a lot of redundant or dead code
- It does not seem like Nextcloud Vue components are being used everywhere, affecting accessibility and overall look and feel as a Nextcloud application
- It has the green dot like someone is editing the file even though the file has been closed for several minutes
- Favoriting a document only pins it on the "My recent documents" page and not in "Home"
- The overall design is not totally consistent with the mockup
- Why is a confetti visual needed? Seems like a waste
- Some JS files could probably be incorporated into components directly
- The hover preview seems a bit much, it's large and kind of distracting
- There are no tests
There was a problem hiding this comment.
As far as I can tell, this file is completely unneeded and was only added "just in case." It should be removed.
| string $appName, | ||
| IRequest $request, | ||
| private OverviewService $service, | ||
| private ?string $userId, |
There was a problem hiding this comment.
This should not be nullable. All of the methods in this controller require a user to be logged-in, so the user ID should never not exist. It also looks visually confusing since it is placed in between two dependency-injected services.
| /** | ||
| * @throws OCSForbiddenException | ||
| */ | ||
| private function requireUser(): void { |
There was a problem hiding this comment.
There are no public endpoints defined in this controller, therefore it is not needed to verify there is a user logged-in at the beginning of each controller method. This method and all the calls to it should be removed.
| /** | ||
| * @NoAdminRequired | ||
| * | ||
| * @throws OCSForbiddenException when called by a guest |
There was a problem hiding this comment.
None of the controller methods can be called by a guest, as they are not given the #[PublicPage] attribute. This doc comment on each of the methods can be removed.
| } | ||
| } | ||
|
|
||
| // Clamp pagination. Server caps at PAGE_SIZE; clients can ask for less but not more. |
There was a problem hiding this comment.
Pagination clamping should not be in the controller, but rather the service.
| #[NoAdminRequired] | ||
| #[NoCSRFRequired] | ||
| public function index(): TemplateResponse { | ||
| $this->initialState->provideInitialState('overview', [ |
There was a problem hiding this comment.
It might make sense to not pass these as initial state, but via the API if the frontend really needs them. Initial state makes sense for configuration values and such, but this I'm not sure.
| * | ||
| * @var list<string> | ||
| */ | ||
| public const ALLOWED_MIMETYPES = [ |
There was a problem hiding this comment.
For now, having a bunch of duplicate MIME types in this class is fine, but in the future I want to do a refactoring so that they are not duplicated across the code base. Just adding this note here so it's on the record.
| */ | ||
| public function getTemplates(string $userId, ?string $query, ?string $type, int $offset, int $limit): array { | ||
| $this->templateManager->setUserId($userId); | ||
| $templates = $this->templateManager->getUser(); |
There was a problem hiding this comment.
Instead of re-implementing the template MIME filtering below, you should specify the type when calling TemplateManager::getUser().
| * | ||
| * @throws NotFoundException | ||
| */ | ||
| public function getTemplateNode(string $userId, int $templateId): File { |
There was a problem hiding this comment.
This seems like an unnecessary wrapper. This method is only called from one place, so could just be inlined.
| * | ||
| * @return array{items: array<int, array<string, mixed>>, nextOffset: ?int} | ||
| */ | ||
| private function getOwnedOrShared(string $userId, ?string $query, ?string $type, int $offset, int $limit, bool $ownedOnly): array { |
There was a problem hiding this comment.
Ideally you wouldn't use private methods to do raw database queries in the service. Better would be to use public APIs. Instead of doing the queries here, maybe it makes sense to do that in the WopiMapper and add the necessary methods?
There was a problem hiding this comment.
A few points from design:
- The pattern of having file types selector in the content is opposite to what both google and microsoft have. Once a document type is selected, in both platforms those are the only files types are displayed in the app content. Recents, shared, etc., are also in the content. That's why it was initially decided to have file types selectors in the navigation.
I don't dislike the top navigation with tabs proposed here, but if we do take this direction, we should remove the left navigation entirely, and only rely on these tabs. We can integrate recents and shared with me in the content as they were planned here. - The “Create new” as a navigation link is a new pattern for us, I think we should stick to having this in the content, or at least as a button at the top of the navigation, depending on what we decide to do with point 1. Also I cannot find a way to actually create a new document once routed to the “Create new” page.
- Some of the components used are not from our components library, these would need to be updated to make use of the ones we have.
- Not design related but flagging anyway: for the display of the list of documents, it was agreed for technical reasons that the same components used in the Files app for would be re-used here. In this pr, new custom ones are used. @juliusknorr and @emberfiend ping for when you're back on this last point as you would know the detailed reasoning.
cc @jancborchardt for further input
|
@karlitschek for the record, @marcoambrosini and I talked at the time of his review and yes, the idea is that any changes needed are made by him directly. |
|
@karlitschek the review was mostly for the record and to take notes, but I also wanted to make you aware of where this might go since you spent time making design decisions that I didn't want to override without raising the concerns first. |
|
I can probably already start making code changes that are unrelated to the visual design of the feature, then. We can coordinate the design improvements since it's a big PR. Might eventually be worth splitting this PR up into different pieces, then they can be merged into this branch or something. |
Adds a new top-level "Office" entry to the app menu that opens a Vue SPA at /apps/richdocuments/overview, providing a single landing page for the user's office work.
Features:
Backend:
Frontend:
Summary
TODO
Checklist