Skip to content

fix(attachments): don't preview attachments when viewer app is disabled - #13491

Open
hamza221 wants to merge 1 commit into
mainfrom
fix/check-viewer
Open

fix(attachments): don't preview attachments when viewer app is disabled#13491
hamza221 wants to merge 1 commit into
mainfrom
fix/check-viewer

Conversation

@hamza221

@hamza221 hamza221 commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

fix #13460

🤖 AI (if applicable)

tests

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

Summary by CodeRabbit

  • Bug Fixes

    • File previews are now hidden when the viewer feature is unavailable for the current user.
    • Previewable attachments continue to display normally when the viewer is enabled.
  • Tests

    • Added coverage for viewer availability and attachment preview behavior.

@hamza221

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 14, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai

coderabbitai Bot commented Aug 14, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The page controller now exposes viewer availability in the initial state. Frontend initialization stores this value in Pinia. AttachmentMixin prevents previewable attachments when the viewer is unavailable. Controller and mixin tests cover the new behavior.

Changes

Viewer availability

Layer / File(s) Summary
Initial state contract
lib/Controller/PageController.php, tests/Unit/Controller/PageControllerTest.php
The controller provides viewer_available, and the test verifies the additional initial-state value.
Frontend state hydration
src/store/mainStore.js, src/store/mainStore/actions.js, src/init.js
The main store defines viewer availability, and initialization loads and stores the controller value.
Attachment preview gating
src/mixins/AttachmentMixin.js, src/tests/unit/mixins/AttachmentMixin.spec.js
Attachment previews return no results when viewer availability is disabled. Tests cover supported attachments and the disabled state.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: 🟡 Moderate · up to ef91e

Attachment rendering can fail when the viewer app is unavailable because preview filtering may access a missing viewer object. The PR is not merge-ready until this case is guarded and covered.

Suggested reviewers: kesselb, christophwurst, gretad

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: preventing attachment previews when the Viewer app is disabled.
Linked Issues check ✅ Passed The changes address issue #13460 by propagating Viewer availability and disabling attachment previews with graceful fallback behavior.
Out of Scope Changes check ✅ Passed All changes support Viewer availability detection, attachment fallback behavior, or related test coverage.
✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/check-viewer

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/mixins/AttachmentMixin.js (1)

25-33: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Guard the Viewer namespace before filtering MIME types.

When viewer_available is absent, src/init.js Line 119 defaults viewerAvailable to true. If OCA.Viewer is absent, Line 33 dereferences an unavailable object and prevents attachment rendering. Check globalThis.OCA?.Viewer before filtering. Add a test with viewerAvailable set to true and global.OCA = {}.

Proposed fix
 		previewableFileInfos() {
-			if (!this.viewerAvailable) {
+			const viewer = globalThis.OCA?.Viewer
+			if (!this.viewerAvailable || !viewer) {
 				return []
 			}

 			return this.fileInfos.filter((fileInfo) => (fileInfo.mime.startsWith('image/')
 				|| fileInfo.mime.startsWith('video/')
 				|| fileInfo.mime.startsWith('audio/')
 				|| fileInfo.mime === 'application/pdf')
-			&& OCA.Viewer.mimetypes.includes(fileInfo.mime))
+			&& viewer.mimetypes.includes(fileInfo.mime))
 		},

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: da5d58cb-f178-445d-9618-d26c2a1e3c60

📥 Commits

Reviewing files that changed from the base of the PR and between c3ee13a and ef91ef2.

📒 Files selected for processing (7)
  • lib/Controller/PageController.php
  • src/init.js
  • src/mixins/AttachmentMixin.js
  • src/store/mainStore.js
  • src/store/mainStore/actions.js
  • src/tests/unit/mixins/AttachmentMixin.spec.js
  • tests/Unit/Controller/PageControllerTest.php

@ChristophWurst ChristophWurst left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How about a check for the global OCA.Viewer? Wouldn't that be easier?

@hamza221

Copy link
Copy Markdown
Contributor Author

How about a check for the global OCA.Viewer? Wouldn't that be easier?

Sure

Comment thread src/mixins/AttachmentMixin.js Outdated
@ChristophWurst

Copy link
Copy Markdown
Member

/backport to stable5.10

@ChristophWurst

Copy link
Copy Markdown
Member

/backport to stable5.11

Signed-off-by: Hamza <hamzamahjoubi221@gmail.com>
@hamza221
hamza221 enabled auto-merge August 14, 2026 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Attachment not shown if Viewer app not installed

3 participants