feat: implement photo rotation functionality for existing attachments#1508
Open
bfritscher wants to merge 1 commit into
Open
feat: implement photo rotation functionality for existing attachments#1508bfritscher wants to merge 1 commit into
bfritscher wants to merge 1 commit into
Conversation
… in ImageDialog by refactoring code from CreateModal
Contributor
Summary by CodeRabbit
WalkthroughThis PR refactors image rotation across the application by extracting utilities, implementing a full rotate-and-persist workflow in ImageDialog, extending the dialog result contract, and updating consumer handlers to support immutable attachment state updates. ChangesImage rotation with client-side persist workflow
Sequence DiagramsequenceDiagram
participant User
participant ImageDialog
participant Canvas
participant API
User->>ImageDialog: Click rotate button
ImageDialog->>Canvas: Load image, rotate 90°
Canvas->>ImageDialog: Return rotated data URL
ImageDialog->>ImageDialog: Mark dirty
User->>ImageDialog: Click close/confirm
ImageDialog->>API: Upload rotated image
API->>ImageDialog: Return new attachment
ImageDialog->>API: Delete original attachment
API->>ImageDialog: Confirm delete
ImageDialog->>User: Close with replace action
Security Considerations
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
frontend/components/Item/ImageDialog.vue (1)
170-178: ⚡ Quick winAdd a MIME allowlist before uploading the rotated file.
At Line 172,
dataUrlToFileoutput is uploaded directly. Add an explicitimage/*allowlist check beforeapi.items.attachments.add(...)to reduce accidental/hostile non-image uploads in this flow.🤖 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 `@frontend/components/Item/ImageDialog.vue` around lines 170 - 178, Before calling api.items.attachments.add, validate the MIME type returned by dataUrlToFile(image.workingDataUrl, fileName); ensure file.type exists and startsWith('image/') (i.e., an image/* allowlist). If the check fails, abort the upload path (return or throw) and surface an error message or log; update the code around getCurrentAttachmentState, dataUrlToFile, and the api.items.attachments.add call to perform this guard and handle the rejection cleanly.
🤖 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 `@frontend/components/Item/CreateModal.vue`:
- Line 238: rotatePhoto can be invoked concurrently for the same index, losing
intermediate rotations; add a per-photo in-flight guard (e.g., a Set or map
keyed by index) in the component state used by rotatePhoto to return early if
that index is already processing, and also bind the rotate button's disabled
state (the Button with `@click.prevent`="rotatePhoto(index)") to that in-flight
flag so rapid clicks are ignored until the rotation promise completes; ensure
the guard is cleared when rotatePhoto's async work finishes or errors so future
rotations are allowed.
In `@frontend/components/Item/ImageDialog.vue`:
- Line 9: The code manually imports composables blobToDataUrl, dataUrlToFile,
and rotateImageDataUrl90Deg from ~/composables/utils which violates the rule
that composables are auto-imported; remove the import statement and rely on the
auto-imported composable functions (blobToDataUrl, dataUrlToFile,
rotateImageDataUrl90Deg) where they are used in ImageDialog.vue so no manual
import remains and any linter errors about undefined symbols are resolved by the
repo's auto-import setup.
- Around line 202-207: The refresh after deleting the old attachment can fail
and leave image.attachmentId pointing at a deleted record, causing subsequent
closes to error; update the post-delete logic in the handler that calls
api.items.get (the block using refreshedData/refreshedError and
refreshedAttachment) to handle refreshedError or missing refreshedAttachment by
clearing image.attachmentId (set to null/undefined) and ensuring the dialog is
closed (call the same close/emit path you use on success) and show the toast
error; this prevents the dialog from getting stuck while still surfacing the
refresh failure.
In `@frontend/composables/utils.ts`:
- Around line 57-59: The current check only validates
mimeType.startsWith("image/") and thus allows SVGs; change the validation in
frontend/composables/utils.ts to use a strict allowlist of raster MIME types
(e.g. image/jpeg, image/png, image/webp, image/gif, image/bmp) instead of any
image/*, perform a case-insensitive comparison against that set for the mimeType
variable, and throw an error with a clear message (e.g. "Invalid mime type,
expected raster image") when not in the allowlist; update the surrounding code
that relies on this check (same function/validation block referencing mimeType)
to use the new allowlist logic.
---
Nitpick comments:
In `@frontend/components/Item/ImageDialog.vue`:
- Around line 170-178: Before calling api.items.attachments.add, validate the
MIME type returned by dataUrlToFile(image.workingDataUrl, fileName); ensure
file.type exists and startsWith('image/') (i.e., an image/* allowlist). If the
check fails, abort the upload path (return or throw) and surface an error
message or log; update the code around getCurrentAttachmentState, dataUrlToFile,
and the api.items.attachments.add call to perform this guard and handle the
rejection cleanly.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0b45999f-2d07-4169-935a-52d0bb7ac622
📒 Files selected for processing (8)
frontend/components/Item/CreateModal.vuefrontend/components/Item/ImageDialog.vuefrontend/components/ui/dialog-provider/utils.tsfrontend/composables/utils.tsfrontend/pages/item/[id]/index.vuefrontend/pages/item/[id]/index/edit.vuefrontend/pages/location/[id]/index/edit.vuefrontend/pages/location/[id]/index/index.vue
| } | ||
| " | ||
| > | ||
| <Button size="icon" type="button" variant="default" @click.prevent="rotatePhoto(index)"> |
Contributor
There was a problem hiding this comment.
Guard rotate against concurrent clicks to avoid lost rotations.
rotatePhoto can run concurrently for the same index; rapid clicks may apply only one effective 90° turn instead of cumulative turns.
🛠️ Suggested fix (per-photo in-flight guard + button disable)
+ const rotatingPhotoIndexes = ref<Set<number>>(new Set());
+
async function rotatePhoto(index: number) {
const photo = form.photos[index];
- if (!photo) {
+ if (!photo || rotatingPhotoIndexes.value.has(index)) {
return;
}
try {
+ rotatingPhotoIndexes.value.add(index);
photo.fileBase64 = await rotateImageDataUrl90Deg(photo.fileBase64);
photo.file = dataUrlToFile(photo.fileBase64, photo.photoName);
} catch (error) {
toast.error(t("components.item.create_modal.toast.rotate_process_failed"));
console.error(error);
+ } finally {
+ rotatingPhotoIndexes.value.delete(index);
}
}- <Button size="icon" type="button" variant="default" `@click.prevent`="rotatePhoto(index)">
+ <Button
+ size="icon"
+ type="button"
+ variant="default"
+ :disabled="rotatingPhotoIndexes.has(index)"
+ `@click.prevent`="rotatePhoto(index)"
+ >Also applies to: 560-573
🤖 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 `@frontend/components/Item/CreateModal.vue` at line 238, rotatePhoto can be
invoked concurrently for the same index, losing intermediate rotations; add a
per-photo in-flight guard (e.g., a Set or map keyed by index) in the component
state used by rotatePhoto to return early if that index is already processing,
and also bind the rotate button's disabled state (the Button with
`@click.prevent`="rotatePhoto(index)") to that in-flight flag so rapid clicks are
ignored until the rotation promise completes; ensure the guard is cleared when
rotatePhoto's async work finishes or errors so future rotations are allowed.
| import type { ItemAttachment } from "~~/lib/api/types/data-contracts"; | ||
| import { AttachmentTypes } from "~~/lib/api/types/non-generated"; | ||
| import { DialogID } from "~/components/ui/dialog-provider/utils"; | ||
| import { blobToDataUrl, dataUrlToFile, rotateImageDataUrl90Deg } from "~/composables/utils"; |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use composable auto-import instead of manual import.
At Line 9, importing from ~/composables/utils violates the frontend composable usage rule for this repo.
As per coding guidelines, "Do not manually import composables from composables/ directory - they are auto-imported".
🤖 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 `@frontend/components/Item/ImageDialog.vue` at line 9, The code manually
imports composables blobToDataUrl, dataUrlToFile, and rotateImageDataUrl90Deg
from ~/composables/utils which violates the rule that composables are
auto-imported; remove the import statement and rely on the auto-imported
composable functions (blobToDataUrl, dataUrlToFile, rotateImageDataUrl90Deg)
where they are used in ImageDialog.vue so no manual import remains and any
linter errors about undefined symbols are resolved by the repo's auto-import
setup.
Comment on lines
+202
to
+207
| const { data: refreshedData, error: refreshedError } = await api.items.get(image.itemId); | ||
| const refreshedAttachment = refreshedData?.attachments.find(attachment => attachment.id === createdAttachment.id); | ||
| if (refreshedError || !refreshedAttachment) { | ||
| toast.error(t("items.toast.failed_update_attachment")); | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
Handle post-delete refresh failure to avoid a stuck dialog state.
At Line 202, if old attachment deletion succeeds but the refresh fails, the function returns without closing, while image.attachmentId still points to a deleted record. Subsequent closes keep failing (Attachment not found), so users can get trapped in the dialog.
Suggested fix
- const { data: refreshedData, error: refreshedError } = await api.items.get(image.itemId);
- const refreshedAttachment = refreshedData?.attachments.find(attachment => attachment.id === createdAttachment.id);
- if (refreshedError || !refreshedAttachment) {
- toast.error(t("items.toast.failed_update_attachment"));
- return;
- }
+ const { data: refreshedData, error: refreshedError } = await api.items.get(image.itemId);
+ const refreshedAttachment = refreshedData?.attachments.find(attachment => attachment.id === createdAttachment.id);
+ if (refreshedError || !refreshedAttachment) {
+ // Fallback to the newly created attachment we already have,
+ // so the dialog can still close and caller can update state.
+ const oldId = image.attachmentId;
+ resetImageState();
+ closeDialog(DialogID.ItemImage, {
+ action: "replace",
+ oldId,
+ attachment: createdAttachment,
+ });
+ toast.success(t("items.toast.attachment_updated"));
+ 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 `@frontend/components/Item/ImageDialog.vue` around lines 202 - 207, The refresh
after deleting the old attachment can fail and leave image.attachmentId pointing
at a deleted record, causing subsequent closes to error; update the post-delete
logic in the handler that calls api.items.get (the block using
refreshedData/refreshedError and refreshedAttachment) to handle refreshedError
or missing refreshedAttachment by clearing image.attachmentId (set to
null/undefined) and ensuring the dialog is closed (call the same close/emit path
you use on success) and show the toast error; this prevents the dialog from
getting stuck while still surfacing the refresh failure.
Comment on lines
+57
to
+59
| if (!mimeType.startsWith("image/")) { | ||
| throw new Error("Invalid mime type, expected image"); | ||
| } |
Contributor
There was a problem hiding this comment.
Restrict image MIME types to a safe allowlist.
Line 57 accepts any image/*, which can unintentionally permit SVG payloads. For attachment photos, constrain to raster formats to reduce XSS/file-content risk.
🔐 Suggested hardening diff
export function dataUrlToFile(dataUrl: string, fileName: string): File {
const parts = dataUrl.split(",");
const mimeMatch = parts[0]?.match(/:(.*?);/);
if (!mimeMatch?.[1]) {
throw new Error("Invalid data URL format");
}
- const mimeType = mimeMatch[1];
- if (!mimeType.startsWith("image/")) {
- throw new Error("Invalid mime type, expected image");
- }
+ const mimeType = mimeMatch[1].toLowerCase();
+ const allowedMimeTypes = new Set([
+ "image/jpeg",
+ "image/png",
+ "image/gif",
+ "image/webp",
+ "image/avif",
+ ]);
+ if (!allowedMimeTypes.has(mimeType)) {
+ throw new Error("Unsupported image mime type");
+ }
const bytes = atob(parts[parts.length - 1] ?? "");
const buffer = new Uint8Array(bytes.length);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (!mimeType.startsWith("image/")) { | |
| throw new Error("Invalid mime type, expected image"); | |
| } | |
| const mimeType = mimeMatch[1].toLowerCase(); | |
| const allowedMimeTypes = new Set([ | |
| "image/jpeg", | |
| "image/png", | |
| "image/gif", | |
| "image/webp", | |
| "image/avif", | |
| ]); | |
| if (!allowedMimeTypes.has(mimeType)) { | |
| throw new Error("Unsupported image mime type"); | |
| } |
🤖 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 `@frontend/composables/utils.ts` around lines 57 - 59, The current check only
validates mimeType.startsWith("image/") and thus allows SVGs; change the
validation in frontend/composables/utils.ts to use a strict allowlist of raster
MIME types (e.g. image/jpeg, image/png, image/webp, image/gif, image/bmp)
instead of any image/*, perform a case-insensitive comparison against that set
for the mimeType variable, and throw an error with a clear message (e.g.
"Invalid mime type, expected raster image") when not in the allowlist; update
the surrounding code that relies on this check (same function/validation block
referencing mimeType) to use the new allowlist logic.
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
1 participant
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.
What type of PR is this?
feat: implement photo rotation functionality for existing attachments in ImageDialog by refactoring code from CreateModal
What this PR does / why we need it:
Adds the ability to rotate photos that have already been uploaded, following up on the earlier create-flow rotation work from #621 / #666.
The backend attachment update API only updates attachment metadata and does not replace the stored file binary, so this change keeps the backend untouched and handles rotation in the frontend by uploading a rotated replacement image and deleting the original attachment.
Changes made:
frontend/components/Item/ImageDialog.vuefrontend/composables/utils.tsFilefrontend/components/Item/CreateModal.vuefrontend/components/ui/dialog-provider/utils.tsreplaceaction in addition todeletefrontend/pages/item/[id]/index.vuefrontend/pages/location/[id]/index/index.vuefrontend/pages/item/[id]/index/edit.vuereplaceresult for item edit attachmentsfrontend/pages/location/[id]/index/edit.vuereplaceresult for location edit attachmentsWhich issue(s) this PR fixes:
Fixes #855
Special notes for your reviewer:
First time contribution, iterated with GPT 5.4 High, and reviewed all the diff.
This intentionally avoids backend changes.
The important implementation detail is that rotating an already-uploaded photo is done as:
Please focus review on:
ImageDialog.vuereplacement flowreplacedialog result handling in the item/location pagesTesting
Manual testing: