✨(frontend) attachments preview#676
Open
jbpenrath wants to merge 1 commit into
Open
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (36)
📝 WalkthroughWalkthroughThis PR adds end-to-end inline attachment preview with server-side MIME validation and security controls. The backend implements a preview endpoint that detects MIME types using python-magic, validates against an allowlist, and returns refusal codes. The frontend provides a modal viewer with provenance sidebar, integrates preview hooks across attachment surfaces, extracts Drive upload state into a reusable hook, and updates localization and dependencies. ChangesAttachment Preview Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
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. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/backend/core/api/openapi.json`:
- Around line 41-64: The OpenAPI operation describing the FilePreview viewer
promises it will refuse unsupported/suspicious MIME types with HTTP 415 but only
declares a 200 response; add a 415 response object to the operation's
"responses" map (alongside the existing "200") describing the refusal (e.g.,
description: "Unsupported media type / preview refused") and optionally include
a JSON error schema or reference to the existing error response schema; update
the operation that uses tag "blob" and path parameter "id" so generated clients
and server stubs will reflect the 415 contract.
In `@src/backend/core/api/viewsets/blob.py`:
- Around line 289-300: The view compares detected_type to the raw declared_type
(from attachment["type"] or blob.content_type) which may include parameters/case
differences; normalize the declared MIME type to a canonical media type (strip
parameters and lowercase, e.g., "type/subtype") before running the
allowlist/suspicious check and before using it for any comparisons—update the
code paths that set declared_type (attachment branch and blob branch using
models.Blob and get_content) so the normalized declared_type is used in the
allowlist comparison and subsequent logic.
- Around line 279-287: The current error handlers in the blob viewset echo
exception messages from utils.get_attachment_from_blob_id and
models.Blob.DoesNotExist back to the caller; change them to return fixed, stable
messages instead (e.g., for the ValueError branch return a 400 with a generic
"invalid blob identifier" message; for the Blob.DoesNotExist branch return a 404
with a generic "blob not found" message), and move the exception details into
internal logging (use the existing logger to log the exception object). Update
the except blocks that reference utils.get_attachment_from_blob_id and
models.Blob.DoesNotExist to stop using str(e) in the Response payload and log
the exception instead while returning the fixed text and appropriate status
constants (status.HTTP_400_BAD_REQUEST and status.HTTP_404_NOT_FOUND).
In `@src/backend/core/mda/draft.py`:
- Line 284: Replace the incorrect eager warning log in the attachment creation
path: change the call to use logger.debug with lazy %-style or comma-separated
formatting and correct tense; e.g., in draft.py where logger.warning(f'Create
{len(new_attachment_ids)} attachments') is used (around the attachment creation
in the function handling new_attachment_ids), update it to logger.debug("Created
%d attachments", len(new_attachment_ids)) so the message is past tense, uses
debug level, and avoids eager interpolation.
In
`@src/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/attachment-item.tsx`:
- Around line 66-72: The keydown handler on AttachmentItem (handleKeyDown)
currently reacts to Enter/Space even when those keys originate from inner action
controls (download/delete/retry); update handleKeyDown to ignore events coming
from interactive descendants by checking the event target (e.g., using e.target
or (e.nativeEvent as KeyboardEvent).target) and returning early if the target is
an interactive element (button, [role="button"], a, input, textarea, select, or
an element matching your action control selectors). Keep the existing
isPreviewable guard and call triggerPreview() only when the key event originates
from the item container itself.
- Around line 54-59: isPreviewable currently ignores the backend can_preview
flag and treats persisted attachments as previewable; update the logic in the
isPreviewable calculation to also require the backend-provided can_preview
property (e.g., attachment.can_preview or driveFile.can_preview) when
determining previewability, and adjust previewableId resolution accordingly so
previewableId is only set when canPreview is true and the attachment is actually
previewable; modify the isPreviewable constant and the previewableId ternary
branches (referencing isPreviewable, canPreview, isAttachment, isDriveFile,
previewableId, attachment, variant) to gate both attachment and drive file
preview behavior on the backend can_preview flag.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9b853589-db3a-4567-a3f1-b8ec1f50dee1
⛔ Files ignored due to path filters (3)
src/frontend/package-lock.jsonis excluded by!**/package-lock.jsonsrc/frontend/src/features/api/gen/blob/blob.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/attachment.tsis excluded by!**/gen/**
📒 Files selected for processing (18)
src/backend/core/api/openapi.jsonsrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets/blob.pysrc/backend/core/enums.pysrc/backend/core/mda/draft.pysrc/backend/core/tests/api/test_blob_preview.pysrc/frontend/package.jsonsrc/frontend/public/locales/common/en-US.jsonsrc/frontend/public/locales/common/fr-FR.jsonsrc/frontend/src/features/forms/components/message-form/attachment-uploader.tsxsrc/frontend/src/features/layouts/components/main/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/attachment-preview-modal/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/attachment-preview-modal/sidebar.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/_index.scsssrc/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/attachment-item.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/index.tsxsrc/frontend/src/features/providers/attachment-preview.tsxsrc/frontend/src/features/utils/attachment-helper/index.ts
jbpenrath
force-pushed
the
feat/attachment-preview
branch
2 times, most recently
from
68ab7d6 to
9ace6d6
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/backend/core/api/openapi.json`:
- Around line 60-63: The 200 response currently says "No response body" but this
endpoint returns inline blob data; update the OpenAPI response object for the
"200" entry under the relevant operation's "responses" to declare binary content
(e.g., add a "content" map with an appropriate media type like
"application/octet-stream" or the actual MIME and set the schema to
{"type":"string","format":"binary"}). Locate the "responses" -> "200" object in
src/backend/core/api/openapi.json and replace the "description": "No response
body" with a description plus a "content" block that advertises the binary
payload so generated clients treat the response as bytes rather than void.
In
`@src/frontend/src/features/layouts/components/thread-view/components/attachment-preview-modal/_index.scss`:
- Line 59: Replace the deprecated CSS declaration "word-break: break-word;" with
the modern property "overflow-wrap: break-word;" in the stylesheet
(_index.scss); locate the rule containing the "word-break: break-word;"
declaration and remove or replace it with "overflow-wrap: break-word;"
(optionally keep "word-break: normal;" if explicit word-break behavior is
needed).
- Line 123: Replace the deprecated CSS declaration "word-break: break-word;"
with the modern property by removing that line and adding "overflow-wrap:
break-word;" in the same selector where "word-break: break-word;" currently
appears so the attachment preview modal text uses overflow-wrap for proper
wrapping; ensure no other occurrences of "word-break: break-word" remain in
_index.scss.
In
`@src/frontend/src/features/layouts/components/thread-view/components/attachment-preview-modal/index.tsx`:
- Around line 199-202: The code opens driveUrl returned by getDriveUrl(file.id)
without validation; parse and validate driveUrl before calling window.open by
using the URL constructor (or equivalent) to ensure it is a well-formed absolute
URL and that url.protocol is either "http:" or "https:" (or your allowed
schemes), reject anything else, and only then call window.open(driveUrl,
"_blank", "noopener,noreferrer"); update the logic in the
attachment-preview-modal where getDriveUrl and window.open are used to perform
this validation and handle invalid/malformed URLs safely (e.g., show an error or
no-op).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4d311607-1478-4775-acdb-64c5df9f9152
⛔ Files ignored due to path filters (4)
src/frontend/package-lock.jsonis excluded by!**/package-lock.jsonsrc/frontend/src/features/api/gen/blob/blob.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/attachment.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/config_retrieve200_driv_e.tsis excluded by!**/gen/**
📒 Files selected for processing (31)
src/backend/core/api/openapi.jsonsrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets/blob.pysrc/backend/core/api/viewsets/config.pysrc/backend/core/enums.pysrc/backend/core/mda/draft.pysrc/backend/core/tests/api/test_blob_preview.pysrc/backend/core/tests/api/test_config.pysrc/backend/core/tests/api/test_drive.pysrc/backend/messages/settings.pysrc/frontend/i18next.config.tssrc/frontend/package.jsonsrc/frontend/public/locales/common/en-US.jsonsrc/frontend/public/locales/common/fr-FR.jsonsrc/frontend/public/pdf.worker.min.mjssrc/frontend/src/features/forms/components/message-form/attachment-uploader.tsxsrc/frontend/src/features/forms/components/message-form/drive-attachment-picker.tsxsrc/frontend/src/features/layouts/components/main/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/attachment-preview-modal/_index.scsssrc/frontend/src/features/layouts/components/thread-view/components/attachment-preview-modal/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/attachment-preview-modal/sidebar-drive-action.tsxsrc/frontend/src/features/layouts/components/thread-view/components/attachment-preview-modal/sidebar.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/_index.scsssrc/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/attachment-item.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/drive-upload-button.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/use-drive-upload.tssrc/frontend/src/features/providers/attachment-preview.tsxsrc/frontend/src/features/providers/config.tsxsrc/frontend/src/features/utils/attachment-helper/index.tssrc/frontend/src/styles/main.scss
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/backend/core/api/serializers.py (1)
646-674:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
can_previewis now part of attachment payloads but is missing on parsed-attachment responses.This serializer change extends the attachment contract, but
MessageSerializer.get_attachments()still builds parsed attachment dicts withoutcan_preview, creating a schema/runtime mismatch for non-draft messages.Suggested fix (in
MessageSerializer.get_attachments)stripped_attachments.append( { "blobId": f"msg_{instance.id}_{index}", "name": attachment["name"], "size": attachment["size"], "type": attachment["type"], "cid": attachment.get("cid"), "sha256": attachment.get("sha256"), + "can_preview": ( + attachment.get("type") in enums.PREVIEWABLE_MIME_TYPES + ), } )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/core/api/serializers.py` around lines 646 - 674, MessageSerializer.get_attachments currently builds parsed attachment dicts but omits the new can_preview field, causing a schema mismatch; update get_attachments to include can_preview for each attachment using the same logic as Attachment serializer (e.g., set can_preview = attachment.content_type in enums.PREVIEWABLE_MIME_TYPES or call the serializer/helper that computes get_can_preview), ensuring parsed-attachment responses include the boolean alongside blobId, name, size, type, sha256, created_at and cid.
♻️ Duplicate comments (3)
src/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/attachment-item.tsx (1)
66-72:⚠️ Potential issue | 🟠 Major | ⚡ Quick winIgnore bubbled keyboard events from inner action controls.
Line 66 handles
Enter/Spaceeven when the key event originates from nested buttons/links, so keyboard actions can unexpectedly open preview.💡 Suggested fix
const handleKeyDown = (e: React.KeyboardEvent<HTMLDivElement>) => { + if (e.target !== e.currentTarget) return; if (!isPreviewable) return; if (e.key === "Enter" || e.key === " ") { e.preventDefault(); triggerPreview(); } };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/attachment-item.tsx` around lines 66 - 72, handleKeyDown is currently triggering preview for Enter/Space even when the keyboard event bubbles from nested interactive controls; update handleKeyDown to ignore bubbled events by verifying the event originated on the container (e.g., compare e.target to e.currentTarget or check the event.composedPath/closest for interactive elements like button, a, input, textarea, select, [role="button"]) before handling; keep the existing isPreviewable and triggerPreview logic but early-return when the origin is an inner action control so only direct key presses on the container open the preview.src/frontend/src/features/layouts/components/thread-view/components/attachment-preview-modal/index.tsx (1)
199-202:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate Drive URL scheme before
window.open.At Line 199-Line 202,
driveUrlis opened directly. Please parse and enforcehttp/httpsbefore opening.🔒 Suggested hardening
+const toSafeHttpUrl = (value: string): string | null => { + try { + const parsed = new URL(value, window.location.origin); + return parsed.protocol === "http:" || parsed.protocol === "https:" + ? parsed.href + : null; + } catch { + return null; + } +}; + const handleDownloadFile = (file?: FilePreviewType) => { if (!file?.url) return; @@ const driveUrl = getDriveUrl(file.id); if (driveUrl) { - window.open(driveUrl, "_blank", "noopener,noreferrer"); + const safeDriveUrl = toSafeHttpUrl(driveUrl); + if (!safeDriveUrl) return; + window.open(safeDriveUrl, "_blank", "noopener,noreferrer"); return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/src/features/layouts/components/thread-view/components/attachment-preview-modal/index.tsx` around lines 199 - 202, Validate the driveUrl before calling window.open: in the attachment-preview-modal component where getDriveUrl(file.id) and window.open(driveUrl, "_blank", ...) are used, parse driveUrl with the URL constructor inside a try/catch and only call window.open if url.protocol is "http:" or "https:"; if parsing fails or protocol is not allowed, do not open the URL and handle it (e.g., log via console/processLogger or show a user-safe error) to prevent unsafe schemes.src/frontend/src/features/layouts/components/thread-view/components/attachment-preview-modal/_index.scss (1)
59-59:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace deprecated
word-break: break-word.At Line 59 and Line 123, use
overflow-wrap: break-word;instead of deprecatedword-break: break-word;.🎨 Suggested fix
.attachment-preview-sidebar__file-name { @@ - word-break: break-word; + overflow-wrap: break-word; @@ .attachment-preview-sidebar__field-value { margin: 0; - word-break: break-word; + overflow-wrap: break-word;Also applies to: 123-123
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/src/features/layouts/components/thread-view/components/attachment-preview-modal/_index.scss` at line 59, Replace the deprecated CSS property usage "word-break: break-word;" with the supported "overflow-wrap: break-word;" at both occurrences in _index.scss (the two places where "word-break: break-word;" appears, e.g., around the styles at lines referenced in the review). Update both instances so the styles use overflow-wrap: break-word; instead of word-break to avoid the deprecated property.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/backend/core/api/viewsets/blob.py`:
- Around line 293-299: Blob lookup with models.Blob.objects.get(id=pk) can raise
a ValidationError for malformed (non-UUID) pk and is currently turning into a
500; validate the pk before querying (e.g., attempt to parse it as a UUID with
uuid.UUID(pk) or explicitly catch
django.core.exceptions.ValidationError/ValueError) and return a 400 Response
with a clear client error message instead of falling through to the generic
exception handler; keep the existing permission check
models.Blob.objects.user_can_access(request.user, blob.id) and subsequent
blob.get_content() flow unchanged once pk is validated.
---
Outside diff comments:
In `@src/backend/core/api/serializers.py`:
- Around line 646-674: MessageSerializer.get_attachments currently builds parsed
attachment dicts but omits the new can_preview field, causing a schema mismatch;
update get_attachments to include can_preview for each attachment using the same
logic as Attachment serializer (e.g., set can_preview = attachment.content_type
in enums.PREVIEWABLE_MIME_TYPES or call the serializer/helper that computes
get_can_preview), ensuring parsed-attachment responses include the boolean
alongside blobId, name, size, type, sha256, created_at and cid.
---
Duplicate comments:
In
`@src/frontend/src/features/layouts/components/thread-view/components/attachment-preview-modal/_index.scss`:
- Line 59: Replace the deprecated CSS property usage "word-break: break-word;"
with the supported "overflow-wrap: break-word;" at both occurrences in
_index.scss (the two places where "word-break: break-word;" appears, e.g.,
around the styles at lines referenced in the review). Update both instances so
the styles use overflow-wrap: break-word; instead of word-break to avoid the
deprecated property.
In
`@src/frontend/src/features/layouts/components/thread-view/components/attachment-preview-modal/index.tsx`:
- Around line 199-202: Validate the driveUrl before calling window.open: in the
attachment-preview-modal component where getDriveUrl(file.id) and
window.open(driveUrl, "_blank", ...) are used, parse driveUrl with the URL
constructor inside a try/catch and only call window.open if url.protocol is
"http:" or "https:"; if parsing fails or protocol is not allowed, do not open
the URL and handle it (e.g., log via console/processLogger or show a user-safe
error) to prevent unsafe schemes.
In
`@src/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/attachment-item.tsx`:
- Around line 66-72: handleKeyDown is currently triggering preview for
Enter/Space even when the keyboard event bubbles from nested interactive
controls; update handleKeyDown to ignore bubbled events by verifying the event
originated on the container (e.g., compare e.target to e.currentTarget or check
the event.composedPath/closest for interactive elements like button, a, input,
textarea, select, [role="button"]) before handling; keep the existing
isPreviewable and triggerPreview logic but early-return when the origin is an
inner action control so only direct key presses on the container open the
preview.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 26ef047b-b02f-43ac-adb6-977f41170957
⛔ Files ignored due to path filters (4)
src/frontend/package-lock.jsonis excluded by!**/package-lock.jsonsrc/frontend/src/features/api/gen/blob/blob.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/attachment.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/config_retrieve200_driv_e.tsis excluded by!**/gen/**
📒 Files selected for processing (31)
src/backend/core/api/openapi.jsonsrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets/blob.pysrc/backend/core/api/viewsets/config.pysrc/backend/core/enums.pysrc/backend/core/tests/api/test_blob_preview.pysrc/backend/core/tests/api/test_config.pysrc/backend/core/tests/api/test_drive.pysrc/backend/messages/settings.pysrc/frontend/eslint.config.mjssrc/frontend/i18next.config.tssrc/frontend/package.jsonsrc/frontend/public/locales/common/en-US.jsonsrc/frontend/public/locales/common/fr-FR.jsonsrc/frontend/public/pdf.worker.min.mjssrc/frontend/src/features/forms/components/message-form/attachment-uploader.tsxsrc/frontend/src/features/forms/components/message-form/drive-attachment-picker.tsxsrc/frontend/src/features/layouts/components/main/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/attachment-preview-modal/_index.scsssrc/frontend/src/features/layouts/components/thread-view/components/attachment-preview-modal/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/attachment-preview-modal/sidebar-drive-action.tsxsrc/frontend/src/features/layouts/components/thread-view/components/attachment-preview-modal/sidebar.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/_index.scsssrc/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/attachment-item.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/drive-upload-button.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/use-drive-upload.tssrc/frontend/src/features/providers/attachment-preview.tsxsrc/frontend/src/features/providers/config.tsxsrc/frontend/src/features/utils/attachment-helper/index.tssrc/frontend/src/styles/main.scss
jbpenrath
force-pushed
the
feat/attachment-preview
branch
2 times, most recently
from
66df5e6 to
b2b3d50
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/frontend/src/features/forms/components/message-form/drive-attachment-picker.tsx (1)
75-95:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix stale closure in
pickcallback dependencies.
pickreadsconfig.DRIVE!.sdk_url/config.DRIVE!.api_urland calls theonPickprop, butuseCallbackonly depends onisDriveDisabled, so it can use staleconfig/onPickafter updates.Proposed fix
- }, [isDriveDisabled]); + }, [config.DRIVE?.api_url, config.DRIVE?.sdk_url, isDriveDisabled, onPick]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/src/features/forms/components/message-form/drive-attachment-picker.tsx` around lines 75 - 95, The pick callback has a stale-closure bug because useCallback only depends on isDriveDisabled while it reads config.DRIVE!.sdk_url / config.DRIVE!.api_url and calls the onPick prop; update the dependency array for the useCallback around pick to include the values it reads (either config or the specific config.DRIVE fields and onPick) so pick always captures the latest config and onPick, or alternatively memoize the sdk_url/api_url and onPick before creating the callback; locate the pick function, the useCallback invocation, and references to config.DRIVE and onPick to apply the change.
♻️ Duplicate comments (2)
src/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/attachment-item.tsx (2)
66-72:⚠️ Potential issue | 🟠 Major | ⚡ Quick winIgnore bubbled keyboard events from inner action controls.
Line 66 currently reacts to Enter/Space even when focus is inside nested action buttons/links, so keyboard actions can also open preview unintentionally.
💡 Suggested fix
const handleKeyDown = (e: React.KeyboardEvent<HTMLDivElement>) => { + if (e.target !== e.currentTarget) return; if (!isPreviewable) return; if (e.key === "Enter" || e.key === " ") { e.preventDefault(); triggerPreview(); } };Also applies to: 112-112
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/attachment-item.tsx` around lines 66 - 72, handleKeyDown is opening the preview when Enter/Space bubbles up from inner action controls; modify handleKeyDown so it ignores events originating inside nested interactive elements by checking the event target before triggering preview (e.g., ensure e.target is the container itself or that (e.target as HTMLElement).closest('button, a, [role="button"], [data-attachment-action]') is null) and only call triggerPreview when that check passes; apply the same guard to the identical handler at the other occurrence referenced (line ~112).
54-59:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGate previewability with backend capability flag.
Line 54 still allows opening preview for persisted attachments that the API marks as non-previewable (
can_preview: false), which can open a modal for unsupported files.💡 Suggested fix
- const isPreviewable = canPreview && (isAttachment(attachment) || isDriveFile(attachment)) && variant !== "error"; + const isServerPreviewable = isAttachment(attachment) ? attachment.can_preview : true; + const isPreviewable = + canPreview && + isServerPreviewable && + (isAttachment(attachment) || isDriveFile(attachment)) && + variant !== "error";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/attachment-item.tsx` around lines 54 - 59, The preview logic allows opening previews for files the backend flagged as non-previewable; update the isPreviewable and previewableId computation to also require the attachment's backend flag (can_preview) to be true (i.e., guard with attachment.can_preview !== false or equivalent) for both Attachment and DriveFile cases, keeping the existing canPreview and variant !== "error" checks and only returning a previewableId when that flag permits previewing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/frontend/public/locales/common/en-US.json`:
- Around line 707-708: Replace the generic message "The email address is
invalid." with the parameterized string "The email {{email}} is invalid." so the
translation preserves the {{email}} template variable; update the JSON entry so
the key and value include "{{email}}" (i.e., restore the original "The email
{{email}} is invalid." string) to provide specific feedback in multi-recipient
contexts.
In
`@src/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/use-drive-upload.ts`:
- Around line 15-19: The hook useDriveUpload initializes driveFileId only once
from driveUploadStore.get(blobId), so when the incoming blobId changes the hook
retains the old driveFileId/state; add an effect that watches blobId and resets
the cached values—call setDriveFileId(driveUploadStore.get(blobId)) and reset
state (e.g. setState("idle") or appropriate initial DriveUploadState) inside a
useEffect with [blobId] so the hook reflects the new blobId; reference
useDriveUpload, driveFileId, driveUploadStore, blobId, setDriveFileId and
setState to locate where to add the effect.
---
Outside diff comments:
In
`@src/frontend/src/features/forms/components/message-form/drive-attachment-picker.tsx`:
- Around line 75-95: The pick callback has a stale-closure bug because
useCallback only depends on isDriveDisabled while it reads config.DRIVE!.sdk_url
/ config.DRIVE!.api_url and calls the onPick prop; update the dependency array
for the useCallback around pick to include the values it reads (either config or
the specific config.DRIVE fields and onPick) so pick always captures the latest
config and onPick, or alternatively memoize the sdk_url/api_url and onPick
before creating the callback; locate the pick function, the useCallback
invocation, and references to config.DRIVE and onPick to apply the change.
---
Duplicate comments:
In
`@src/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/attachment-item.tsx`:
- Around line 66-72: handleKeyDown is opening the preview when Enter/Space
bubbles up from inner action controls; modify handleKeyDown so it ignores events
originating inside nested interactive elements by checking the event target
before triggering preview (e.g., ensure e.target is the container itself or that
(e.target as HTMLElement).closest('button, a, [role="button"],
[data-attachment-action]') is null) and only call triggerPreview when that check
passes; apply the same guard to the identical handler at the other occurrence
referenced (line ~112).
- Around line 54-59: The preview logic allows opening previews for files the
backend flagged as non-previewable; update the isPreviewable and previewableId
computation to also require the attachment's backend flag (can_preview) to be
true (i.e., guard with attachment.can_preview !== false or equivalent) for both
Attachment and DriveFile cases, keeping the existing canPreview and variant !==
"error" checks and only returning a previewableId when that flag permits
previewing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d0ed39e0-a73a-4cc1-822a-b5f6c99014b5
⛔ Files ignored due to path filters (3)
src/frontend/package-lock.jsonis excluded by!**/package-lock.jsonsrc/frontend/src/features/api/gen/blob/blob.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/config_retrieve200_driv_e.tsis excluded by!**/gen/**
📒 Files selected for processing (36)
src/backend/core/api/openapi.jsonsrc/backend/core/api/viewsets/blob.pysrc/backend/core/api/viewsets/config.pysrc/backend/core/enums.pysrc/backend/core/tests/api/test_attachments.pysrc/backend/core/tests/api/test_blob_preview.pysrc/backend/core/tests/api/test_config.pysrc/backend/core/tests/api/test_drive.pysrc/backend/messages/settings.pysrc/frontend/eslint.config.mjssrc/frontend/i18next.config.tssrc/frontend/package.jsonsrc/frontend/public/locales/common/en-US.jsonsrc/frontend/public/locales/common/fr-FR.jsonsrc/frontend/public/pdf.worker.min.mjssrc/frontend/src/features/forms/components/message-form/_index.scsssrc/frontend/src/features/forms/components/message-form/attachment-uploader.tsxsrc/frontend/src/features/forms/components/message-form/drive-attachment-picker.tsxsrc/frontend/src/features/layouts/components/main/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/attachment-preview-modal/_index.scsssrc/frontend/src/features/layouts/components/thread-view/components/attachment-preview-modal/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/attachment-preview-modal/sidebar-drive-action.tsxsrc/frontend/src/features/layouts/components/thread-view/components/attachment-preview-modal/sidebar.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/_index.scsssrc/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/attachment-item.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/drive-upload-button.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-attachment-list/use-drive-upload.tssrc/frontend/src/features/layouts/components/thread-view/components/thread-event/_index.scsssrc/frontend/src/features/layouts/components/thread-view/components/thread-message/_index.scsssrc/frontend/src/features/providers/attachment-preview.tsxsrc/frontend/src/features/providers/config.tsxsrc/frontend/src/features/ui/components/contact-chip/_index.scsssrc/frontend/src/features/utils/attachment-helper/index.tssrc/frontend/src/features/utils/mail-helper.tsxsrc/frontend/src/styles/main.scss
Use the Preview component to allow user to preview message attachment from a thread.
jbpenrath
force-pushed
the
feat/attachment-preview
branch
from
b2b3d50 to
6834740
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Use the Preview component to allow user to preview message attachment from a thread.
Summary by CodeRabbit
New Features
Updates
Bug Fixes