Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 36 additions & 7 deletions frontend/src/components/assets/useAssetActions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { useRouter } from 'vue-router'
import { dialog, toast, useCall, type DropdownOption } from 'frappe-ui'
import type { Asset } from '@/types'
import { serverMessage } from '@/lib/format'
import { isConvertibleStill } from '@/lib/fileType'
import { useDownload } from '@/composables/useDownload'
import { useVersionUpload } from '@/composables/useVersionUpload'

Expand Down Expand Up @@ -50,7 +51,7 @@ interface AssetParams {
*/
export function useAssetActions(asset: Ref<Asset>, ctx: AssetActionsContext): DropdownOption[] {
const router = useRouter()
const { downloadOne } = useDownload()
const { downloadOne, downloadConverted } = useDownload()
const { openVersionUpload } = useVersionUpload()

const isReady = () => asset.value.status === 'Ready'
Expand Down Expand Up @@ -187,19 +188,47 @@ export function useAssetActions(asset: Ref<Asset>, ctx: AssetActionsContext): Dr
},
]

const downloadOption: DropdownOption = isConvertibleStill(
asset.value.file_type,
asset.value.file_name,
)
? {
label: 'Download',
icon: 'lucide-download',
condition: isReady,
submenu: [
{
label: 'Original',
icon: 'lucide-download',
onClick: () => downloadOne(asset.value.name, asset.value.file_name),
},
{
label: 'JPEG',
icon: 'lucide-image',
onClick: () => downloadConverted(asset.value.name, asset.value.file_name, 'jpeg'),
},
{
label: 'PNG',
icon: 'lucide-image',
onClick: () => downloadConverted(asset.value.name, asset.value.file_name, 'png'),
},
],
}
: {
label: 'Download',
icon: 'lucide-download',
condition: isReady,
onClick: () => downloadOne(asset.value.name, asset.value.file_name),
}

return [
{
label: 'Review',
icon: 'lucide-play',
condition: isReady,
onClick: () => router.push(`/review/${asset.value.name}`),
},
{
label: 'Download',
icon: 'lucide-download',
condition: isReady,
onClick: () => downloadOne(asset.value.name, asset.value.file_name),
},
downloadOption,
{
label: 'Copy review link',
icon: 'lucide-link',
Expand Down
55 changes: 48 additions & 7 deletions frontend/src/components/review/ReviewHeader.vue
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,20 @@
/>
</div>

<Dropdown
v-if="review.isGuest.value && canPickFormat"
:options="downloadFormatOptions"
align="end"
>
<Button
variant="subtle"
icon-left="lucide-download"
label="Download"
:loading="isDownloading"
/>
</Dropdown>
<Button
v-if="review.isGuest.value"
v-else-if="review.isGuest.value"
variant="subtle"
icon-left="lucide-download"
label="Download"
Expand Down Expand Up @@ -78,6 +90,7 @@ import {
} from 'frappe-ui'
import type { ReviewAsset } from '@/types'
import { assetStatusTheme } from '@/lib/status'
import { isConvertibleStill } from '@/lib/fileType'
import { useDownload } from '@/composables/useDownload'
import { useReview } from '@/composables/useReview'
import YoutubeIcon from '@/components/common/YoutubeIcon.vue'
Expand All @@ -97,7 +110,33 @@ const emit = defineEmits<{
const router = useRouter()
const review = useReview()
const shareOpen = ref(false)
const { downloadOne, isDownloading } = useDownload(review.token)
const { downloadOne, downloadConverted, isDownloading } = useDownload(review.token)

const canPickFormat = computed(() =>
isConvertibleStill(props.asset.file_type, props.asset.file_name),
)

const originalExt = computed(
() => props.asset.file_name.match(/\.([^.]+)$/)?.[1]?.toUpperCase() ?? 'file',
)

const downloadFormatOptions = computed<DropdownOption[]>(() => [
{
label: `Original (${originalExt.value})`,
icon: 'lucide-download',
onClick: () => downloadOne(props.asset.name, props.asset.file_name),
},
{
label: 'JPEG',
icon: 'lucide-image',
onClick: () => downloadConverted(props.asset.name, props.asset.file_name, 'jpeg'),
},
{
label: 'PNG',
icon: 'lucide-image',
onClick: () => downloadConverted(props.asset.name, props.asset.file_name, 'png'),
},
])

const publicReview = computed({
get: () => props.asset.is_public_review === 1,
Expand All @@ -111,11 +150,13 @@ const shareUrl = computed(() => {

const menuOptions = computed<DropdownOption[]>(() => {
const options: DropdownOption[] = [
{
label: 'Download',
icon: 'lucide-download',
onClick: () => downloadOne(props.asset.name, props.asset.file_name),
},
canPickFormat.value
? { label: 'Download', icon: 'lucide-download', submenu: downloadFormatOptions.value }
: {
label: 'Download',
icon: 'lucide-download',
onClick: () => downloadOne(props.asset.name, props.asset.file_name),
},
{
label: 'New version',
icon: 'lucide-upload',
Expand Down
21 changes: 20 additions & 1 deletion frontend/src/composables/useDownload.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ import { ref } from 'vue'
import { call, toast } from 'frappe-ui'
import type { ViewUrlResponse } from '@/types'

export type DownloadFormat = 'jpeg' | 'png'

function triggerDownload(url: string, fileName: string) {
const a = document.createElement('a')
a.href = url
Expand All @@ -11,6 +13,10 @@ function triggerDownload(url: string, fileName: string) {
document.body.removeChild(a)
}

function convertedName(fileName: string, format: DownloadFormat) {
return fileName.replace(/\.[^.]+$/, '') + (format === 'jpeg' ? '.jpg' : '.png')
}

/**
* Presigned R2 downloads. With a guest review `token` the request goes through
* `review_api.get_guest_download_url`; otherwise `api.get_download_url`.
Expand Down Expand Up @@ -38,6 +44,19 @@ export function useDownload(token?: string | null) {
}
}

function downloadConverted(assetName: string, fileName: string, format: DownloadFormat) {
const params = new URLSearchParams({ asset_name: assetName, format })
if (token) params.set('token', token)
const method = token
? 'vms.review_api.download_guest_converted_asset'
: 'vms.api.download_converted_asset'
toast.info(`Preparing ${format.toUpperCase()}…`)
triggerDownload(
`/api/v2/method/${method}?${params.toString()}`,
convertedName(fileName, format),
)
Comment on lines +54 to +57

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Conversion Errors Download as Images

When conversion fails because the token expired, the asset is oversized, R2 is unavailable, or decoding fails, this anchor-based request cannot inspect the HTTP response or show an error toast. The browser may download the Frappe error response under the requested .jpg or .png filename after telling the user the conversion is being prepared, leaving them with a corrupt-looking file and no actionable feedback.

Knowledge Base Used:

Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/composables/useDownload.ts
Line: 54-57

Comment:
**Conversion Errors Download as Images**

When conversion fails because the token expired, the asset is oversized, R2 is unavailable, or decoding fails, this anchor-based request cannot inspect the HTTP response or show an error toast. The browser may download the Frappe error response under the requested `.jpg` or `.png` filename after telling the user the conversion is being prepared, leaving them with a corrupt-looking file and no actionable feedback.

**Knowledge Base Used:**
- [Platform APIs and operations](https://app.greptile.com/bwh-tech/-/custom-context/knowledge-base/bwhtech/vms/-/docs/platform-api-and-operations.md)
- [Media review and annotation](https://app.greptile.com/bwh-tech/-/custom-context/knowledge-base/bwhtech/vms/-/docs/review-and-annotation.md)

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Fix in Claude Code Fix in Codex

}

async function downloadMany(assets: { name: string; file_name: string }[]) {
isDownloading.value = true
let failed = 0
Expand All @@ -58,5 +77,5 @@ export function useDownload(token?: string | null) {
}
}

return { downloadOne, downloadMany, isDownloading }
return { downloadOne, downloadConverted, downloadMany, isDownloading }
}
12 changes: 12 additions & 0 deletions frontend/src/lib/fileType.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,18 @@
export type FileKind = 'video' | 'image' | 'audio' | 'file'

export const RAW_EXTENSIONS = ['arw', 'cr2', 'cr3', 'dng', 'nef', 'orf', 'raf', 'rw2']
export const HEIC_EXTENSIONS = ['heic', 'heif']

function extensionOf(fileName?: string | null): string {
return fileName?.toLowerCase().match(/\.([^.]+)$/)?.[1] ?? ''
}

export function isConvertibleStill(fileType?: string | null, fileName?: string | null): boolean {
const ext = extensionOf(fileName)
if (RAW_EXTENSIONS.includes(ext) || HEIC_EXTENSIONS.includes(ext)) return true
const type = fileType ?? ''
return type === 'image/heic' || type === 'image/heif' || type.startsWith('image/x-')
}

export interface FileKindStyle {
icon: string
Expand Down
1 change: 1 addition & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ dependencies = [
# "frappe~=16.0.0" # Installed and managed by bench.
"boto3>=1.35.0",
"rawpy>=0.24.0",
"pillow-heif>=0.18.0",
]

[deploy.dependencies.apt]
Expand Down
16 changes: 16 additions & 0 deletions vms/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import frappe
import requests
from frappe import _
from frappe.rate_limiter import rate_limit
from frappe.utils import cint

from vms.permissions import require_vms_access
Expand Down Expand Up @@ -548,6 +549,21 @@ def get_download_url(asset_name: str):
return {"url": url}


@frappe.whitelist()
@rate_limit(key="asset_name", limit=30, seconds=60, methods=["GET"])
def download_converted_asset(asset_name: str, format: str):
require_vms_access()

asset = frappe.get_doc("VMS Asset", asset_name)

if not asset.r2_key:
frappe.throw(_("Asset has no R2 key"))

from vms.image_export import serve_converted_download

serve_converted_download(asset, format)


@frappe.whitelist()
def move_asset(asset_name: str, target_project: str):
"""Move an asset to a different project (or from Inbox to a project)."""
Expand Down
137 changes: 137 additions & 0 deletions vms/image_export.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
import io
import os
import tempfile

import frappe
import requests
from frappe import _
from PIL import Image, ImageOps

from vms.r2 import generate_presigned_view_url
from vms.raw_images import is_raw, open_raw_preview

HEIC_EXTENSIONS = frozenset({".heic", ".heif"})

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 HEIC uploads remain blocked

The conversion code supports HEIC and HEIF, but the default upload extension allowlist still omits heic and heif. On normally configured sites, the upload API rejects these files before they can reach the new conversion feature. Add both extensions to the default and provide an upgrade path for existing VMS Settings records.

Knowledge Base Used: Platform APIs and operations

Prompt To Fix With AI
This is a comment left during a code review.
Path: vms/image_export.py
Line: 13

Comment:
**HEIC uploads remain blocked**

The conversion code supports HEIC and HEIF, but the default upload extension allowlist still omits `heic` and `heif`. On normally configured sites, the upload API rejects these files before they can reach the new conversion feature. Add both extensions to the default and provide an upgrade path for existing `VMS Settings` records.

**Knowledge Base Used:** [Platform APIs and operations](https://app.greptile.com/bwh-tech/-/custom-context/knowledge-base/bwhtech/vms/-/docs/platform-api-and-operations.md)

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Fix in Claude Code Fix in Codex

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in dc1a55f — added heic,heif to the default allowed_extensions in vms_settings.json and a vms.patches.allow_heic_uploads patch that extends existing VMS Settings records (inserts before arw, skips if already present).

HEIC_MIMES = frozenset({"image/heic", "image/heif", "image/heic-sequence", "image/heif-sequence"})

TARGET_FORMATS = {
"jpeg": ("JPEG", "image/jpeg", "jpg"),
"jpg": ("JPEG", "image/jpeg", "jpg"),
"png": ("PNG", "image/png", "png"),
}

JPEG_QUALITY = 90
_WHITE = (255, 255, 255)

MAX_SOURCE_BYTES = 300 * 1024 * 1024


def _extension(file_name: str | None) -> str:
if not file_name:
return ""
return os.path.splitext(file_name)[1].lower()


def is_heic(file_type: str | None = None, file_name: str | None = None) -> bool:
if file_type in HEIC_MIMES:
return True
return _extension(file_name) in HEIC_EXTENSIONS


def is_convertible_still(file_type: str | None = None, file_name: str | None = None) -> bool:
return is_raw(file_type, file_name) or is_heic(file_type, file_name)


def converted_file_name(file_name: str, ext: str) -> str:
base = os.path.splitext(file_name)[0] or file_name
return f"{base}.{ext}"


def _register_heif():
try:
from pillow_heif import register_heif_opener
except ImportError:
frappe.throw(_("HEIC support is not installed on the server (pillow-heif)."))
register_heif_opener()


def _render(src_path: str, file_type: str | None, file_name: str | None) -> Image.Image:
if is_raw(file_type, file_name):
return open_raw_preview(src_path)

if is_heic(file_type, file_name):
_register_heif()

img = Image.open(src_path)
img.load()
return ImageOps.exif_transpose(img)


def _has_alpha(img: Image.Image) -> bool:
return img.mode in ("RGBA", "LA", "PA") or "transparency" in img.info


def _encode(img: Image.Image, pil_format: str) -> bytes:
buf = io.BytesIO()
if pil_format == "JPEG":
if _has_alpha(img):
rgba = img.convert("RGBA")
flattened = Image.new("RGB", rgba.size, _WHITE)
flattened.paste(rgba, mask=rgba.split()[-1])
img = flattened
else:
img = img.convert("RGB")
img.save(buf, "JPEG", quality=JPEG_QUALITY, optimize=True, progressive=True)
else:
img = img.convert("RGBA") if _has_alpha(img) else img.convert("RGB")
img.save(buf, "PNG", optimize=True)
return buf.getvalue()


def _download_source(r2_key: str, dest_path: str):
resp = requests.get(generate_presigned_view_url(r2_key), stream=True, timeout=120)
resp.raise_for_status()
# nosemgrep: frappe-semgrep-rules.rules.security.frappe-security-file-traversal
with open(dest_path, "wb") as f:
for chunk in resp.iter_content(chunk_size=1024 * 1024):
f.write(chunk)


def convert_asset_image(asset, target_format: str) -> tuple[bytes, str, str]:
fmt = TARGET_FORMATS.get((target_format or "").lower())
if not fmt:
frappe.throw(_("Unsupported download format."))
pil_format, mime, ext = fmt

if not is_convertible_still(asset.file_type, asset.file_name):
frappe.throw(_("This file is not available as JPEG or PNG."))

if asset.file_size and asset.file_size > MAX_SOURCE_BYTES:
frappe.throw(_("This file is too large to convert on download. Download the original instead."))

with tempfile.TemporaryDirectory(prefix="vms_export_") as tmp:
src_path = os.path.join(tmp, f"source{_extension(asset.file_name) or '.bin'}")
_download_source(asset.r2_key, src_path)
img = _render(src_path, asset.file_type, asset.file_name)
data = _encode(img, pil_format)

return data, mime, converted_file_name(asset.file_name, ext)


def serve_converted_download(asset, target_format: str):
data, mime, out_name = convert_asset_image(asset, target_format)
Comment thread
greptile-apps[bot] marked this conversation as resolved.

from vms.deletion import _create_audit_log

_create_audit_log(
action="Download",
asset_name=asset.name,
file_name=out_name,
file_type=mime,
project=asset.project,
file_size=len(data),
)

frappe.local.response.filename = out_name
frappe.local.response.filecontent = data
frappe.local.response.content_type = mime
frappe.local.response.type = "download"
Loading
Loading