Skip to content

feat: show preview thumbnails in the list view - #83

Closed
karlitschek wants to merge 1 commit into
fix/recent-files-sort-orderfrom
feat/list-view-thumbnails
Closed

feat: show preview thumbnails in the list view#83
karlitschek wants to merge 1 commit into
fix/recent-files-sort-orderfrom
feat/list-view-thumbnails

Conversation

@karlitschek

Copy link
Copy Markdown
Member

Fill the NcListItem icon slot with a small (40px, 80px requested for hidpi) lazy-loaded preview thumbnail, using the same etag cache-busting as the grid view. getPreviewUrl() gained a size parameter so the grid keeps its 300px previews. Falls back to a document icon when the preview request fails; mimeFallback=true keeps that case rare.

Includes rebuilt assets.

🤖 AI (if applicable)

  • The content of this PR was partly or fully generated using AI

Fill the NcListItem icon slot with a small (40px, 80px requested for
hidpi) lazy-loaded preview thumbnail, using the same etag cache-busting
as the grid view. getPreviewUrl() gained a size parameter so the grid
keeps its 300px previews. Falls back to a document icon when the
preview request fails; mimeFallback=true keeps that case rare.

Includes rebuilt assets.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@karlitschek
karlitschek requested a review from moodyjmz July 23, 2026 12:55
@karlitschek

Copy link
Copy Markdown
Member Author

@emberfiend

@moodyjmz

Copy link
Copy Markdown
Contributor

Stacked on #79's branch, so it inherits that PR's problems. Base branch is fix/recent-files-sort-order (#79), not main — once #79 is closed and redone against current main, this needs a full rebase too (the OfficeOverview.vue here still has the pre-refactor layout; main's test harness for this file doesn't exist on this branch at all). DCO is also failing (no Signed-off-by:).

Details

The feature diff itself is small and clean — no blockers in the code:

  • Security: clean. getPreviewUrl()'s params are either hardcoded literals (size: 80/300) or server-supplied (fileid, etag); generateUrl encodes substitutions. Nothing user-controlled reaches the URL.
  • a11y: the list thumbnail's alt="" is the correct call, not a regression — NcListItem's :name prop already renders the filename as accessible text, so an empty alt avoids double-announcing it to a screen reader.
  • Performance: real but minor tradeoff — list view goes from zero preview requests to matching grid view's cost. loading="lazy" genuinely mitigates it (200 rows is far taller than any viewport), so this isn't a "200 requests on load" problem. Worth knowing this is a deliberate tradeoff, not a regression to fix.
  • The shared failedPreviews cache (keyed only by fileid, now used by both the 300px grid and 80px list requests) can in theory let a failure at one size suppress the thumbnail at the other after a view switch. In practice this is close to moot — Nextcloud's /core/preview with mimeFallback=true clamps oversized requests and returns a 200 with a mimetype icon rather than erroring, so @error only fires on failures that are size-independent anyway (missing provider, permissions). Worth a one-line comment in the code; not worth blocking on.

Test coverage — the real gap. No existing test (grid or list, old or new) covers getPreviewUrl(), the failedPreviews fallback-to-icon behaviour, or preview rendering at all — and this branch doesn't even have the OfficeOverview.spec.ts harness that exists on main. Once rebased, worth adding:

  1. List view renders the thumbnail <img> with size=80 reflected in the URL (x=80&y=80).
  2. getPreviewUrl()'s URL shape, including the 6-char etag v param, for a given size.
  3. @error flips failedPreviews[fileid] and swaps to the document-icon fallback — for both grid and list.

Suggested path: hold until #79 is resolved, then rebase this onto the redone base, pull in OfficeOverview.spec.ts, add the three tests above, and sign off the commit.

@moodyjmz

Copy link
Copy Markdown
Contributor

Superseded by #87 — rebased onto current main (this PR's base predated the file-filtering refactor), with Frank's original diagnosis and feature credited via Co-authored-by:, the shared-failedPreviews-across-sizes issue fixed via a dedicated FilePreview component, and tests added. Will close this once #87 merges.

moodyjmz added a commit that referenced this pull request Aug 10, 2026
Adds four rules covering what came up while redoing #79/#83 this
session: reuse existing design tokens over hardcoded values, extract
components only for genuine multi-site duplication (as its own
commit unless the component is new to the feature), prefer
@nextcloud/vue over hand-rolled markup, and structure code for
testability without extracting solely to reach a test.

Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
moodyjmz added a commit that referenced this pull request Aug 10, 2026
Every other rule in this file explains its motivating incident in the
commit message or PR body, never inside the instructional text itself
— #79, #83, #84, and #100's own diagnosis are all discussed there, not
quoted in AGENTS.md. "see #100" broke that pattern and adds nothing:
the DAV SEARCH orderby/limit example is already self-contained, and a
bare PR number means nothing to a future reader without the context
this conversation had.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
@moodyjmz

Copy link
Copy Markdown
Contributor

Superseded by #87, now merged as bf7e76b.

Attribution carried over: the feature commit on main is 20eb90e, with Co-authored-by: Frank Karlitschek.

#87 is the version that landed — rebased onto current main, DCO signed off, and with unit tests (src/components/FilePreview.spec.ts, plus additions to src/views/OfficeOverview.spec.ts).

Closing this one.

@moodyjmz moodyjmz closed this Aug 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants