Skip to content

feat: only request thumnail if server support is guaranteed #874

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
AlexAndBear merged 13 commits into from
Jul 25, 2025
Merged

feat: only request thumnail if server support is guaranteed #874

AlexAndBear merged 13 commits into from
Jul 25, 2025

Conversation

@AlexAndBear
Copy link
Contributor

@AlexAndBear AlexAndBear commented Jun 27, 2025 •
edited
Loading

Description

Related Issue

How Has This Been Tested?

  • test environment:
  • test case 1:
  • test case 2:
  • ...

Types of changes

  • Bugfix
  • Enhancement (a change that doesn't break existing code or deployments)
  • Breaking change (a modification that affects current functionality)
  • Technical debt (addressing code that needs refactoring or improvements)
  • Tests (adding or improving tests)
  • Documentation (updates or additions to documentation)
  • Maintenance (like dependency updates or tooling adjustments)

Copilot AI review requested due to automatic review settings June 27, 2025 11:12
Copilot AI reviewed Jun 27, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR ensures that thumbnails are only requested when the server explicitly indicates preview support.

  • Removes server-side capability checks in PreviewService in favor of a hasPreview flag on resources
  • Adds a hasPreview property to the resource interface and populates it from WebDAV props
  • Cleans up test setup by dropping the unused capabilityStore

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
packages/web-pkg/tests/unit/services/previewService.spec.ts Removed capabilityStore injection from test wrapper
packages/web-pkg/src/services/preview/previewService.ts Simplified loadPreview to rely on resource.hasPreview
packages/web-client/src/webdav/constants/dav.ts Introduced HasPreview DAV property
packages/web-client/src/helpers/resource/types.ts Added optional hasPreview method to Resource
packages/web-client/src/helpers/resource/functions.ts Implemented hasPreview in buildResource
Comments suppressed due to low confidence (2)

packages/web-pkg/tests/unit/services/previewService.spec.ts:211

  • Tests no longer mock capabilityStore, but also need to stub resource.hasPreview. Add a default hasPreview: () => true (or false) in the test wrapper to cover the new preview logic.
      authStore

packages/web-client/src/helpers/resource/types.ts:73

  • [nitpick] hasPreview is critical for preview logic, so it should be a required method (hasPreview(): boolean) rather than optional to prevent missing implementations.
  hasPreview?(): boolean

packages/web-pkg/src/services/preview/previewService.ts Outdated
resource.type !== 'folder' &&
resource.extension &&
resource.canDownload() &&
resource.hasPreview()
Copy link

Copilot AI Jun 27, 2025

Choose a reason for hiding this comment

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

Calling resource.hasPreview() without guarding against hasPreview being undefined can cause a runtime error. Consider using resource.hasPreview?.() or making the method non-optional.

Suggested change
resource.hasPreview()
resource.hasPreview?.()

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@AlexAndBear AlexAndBear Jun 27, 2025

Choose a reason for hiding this comment

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

we need to implement it for the other interfaces as well

@AlexAndBear
Copy link
Contributor Author

AlexAndBear commented Jun 27, 2025

Blocked by opencloud-eu/opencloud#1133

@AlexAndBear AlexAndBear requested a review from JammingBen July 23, 2025 09:32
AlexAndBear
AlexAndBear commented Jul 23, 2025
View reviewed changes
packages/web-client/src/helpers/space/functions.ts
mimeType: space.spaceImageData.file.mimeType,
type: 'file',
webDavPath: urlJoin(space.webDavPath, '.space', space.spaceImageData.name),
hasPreview: () => true,
Copy link
Contributor Author

@AlexAndBear AlexAndBear Jul 23, 2025

Choose a reason for hiding this comment

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

this correct (space images always have preview)

@AlexAndBear
Copy link
Contributor Author

AlexAndBear commented Jul 23, 2025

Needs OPENCLOUD_COMMIT_ID bump after opencloud-eu/opencloud#1257 is merged

JammingBen
JammingBen reviewed Jul 25, 2025
View reviewed changes
Copy link
Contributor

@JammingBen JammingBen left a comment

Choose a reason for hiding this comment

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

2 nitpicks, rest LGTM 👍

AlexAndBear and others added 2 commits July 25, 2025 11:53
@AlexAndBear @JammingBen
Update previewService.ts
5b08f40 
Co-authored-by: Jannik Stehle <50302941+JammingBen@users.noreply.github.com>
@AlexAndBear
rm unused var
79091fc
@AlexAndBear AlexAndBear requested a review from JammingBen July 25, 2025 10:32
JammingBen
JammingBen approved these changes Jul 25, 2025
View reviewed changes
Copy link
Contributor

@JammingBen JammingBen left a comment

Choose a reason for hiding this comment

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

👍 Type checks need a fix though.

@AlexAndBear AlexAndBear merged commit b4825ca into main Jul 25, 2025
23 checks passed
@AlexAndBear AlexAndBear deleted the issues/864 branch July 25, 2025 11:06
@openclouders openclouders mentioned this pull request Jul 25, 2025
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@JammingBen JammingBen JammingBen approved these changes

Assignees

No one assigned

Labels

Type:Enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Request preview/thumbnail only when available

3 participants

@AlexAndBear @JammingBen