Conversation
Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
Introduced a new test for verifying orientation without `checkOrientation` to ensure browser-native handling works correctly. Updated the Vitest configuration to include Firefox testing. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
Removed the `checkOrientation` option, simplifying orientation handling by relying on browser-native EXIF processing. Adjusted related code, utilities, tests, and documentation to reflect this change. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
Updated the README to reflect changes in version 2.0.0, including the removal of the `checkOrientation` option and the adjusted default for `convertTypes`. Clarified API updates and added details on dropped Internet Explorer support. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRemoves Changes
Sequence Diagram(s)sequenceDiagram
participant U as User / Client
participant FR as FileReader
participant Util as resetOrientation
participant IMG as HTMLImageElement
participant CAN as Canvas
participant AS as Async EXIF Stripper
participant CB as Success Callback
U->>FR: provide File (readAsArrayBuffer)
FR->>Util: pass ArrayBuffer
Util-->>FR: returns { orientation?, exifData? }
FR->>IMG: createObjectURL + set src
IMG-->>CAN: onload -> draw({ naturalWidth, naturalHeight })
CAN-->>CAN: draw image at 0,0,width,height (no rotation)
CAN->>CB: toBlob / output blob
alt strictFallback && JPEG && retainExif == false
U->>AS: request async EXIF strip on original file
AS-->>CB: returns stripped Blob
end
CAN-->>CB: invoke success callback with blob
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Updated the `pretest` script in `package.json` to install all Playwright browsers instead of only Chromium. This change ensures that tests are properly prepared for multiple browser environments. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/index.js`:
- Around line 148-155: The call to resetAndGetOrientation discards its return
value, so rename the function to reflect its side-effect-only use (e.g.,
resetOrientation or resetExifOrientation) and update its definition and all
callers accordingly; specifically, rename the function symbol
resetAndGetOrientation to resetOrientation (or chosen name) in the declaration
and replace calls such as the one in src/index.js where it’s invoked before
getExif so the code reads resetOrientation(target.result); also update any
exports/imports and tests that reference resetAndGetOrientation to the new name
to keep the API consistent.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Line 86: The pretest script currently runs "playwright install" which installs
all browser engines; change the NPM script named "pretest" in package.json to
explicitly install only the needed browsers (chromium and firefox) by updating
its value to run the narrower command (e.g., "playwright install chromium
firefox") so CI avoids downloading WebKit unnecessarily.
Renamed the `resetAndGetOrientation` function to `resetOrientation` for improved clarity and better alignment with its purpose. Adjusted all relevant references in the codebase to reflect the updated function name. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
Enhanced strict fallback logic to ensure EXIF data is stripped consistently across browsers when returning the original file. Added tests to validate updated output behavior and compatibility with WebKit. Improved browser-specific support in the testing configuration. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
Added `workflow_dispatch` to the GitHub Actions configuration for manual workflow runs. Updated Playwright to install Chromium in `pretest` and all browsers in `pretest:full`, optimizing browser-specific and full matrix testing. Adjusted `test` scripts to support distinct browser contexts. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
Added details on consistent EXIF data stripping across browsers and expanded the browser test matrix to include Firefox and WebKit. Documented these improvements under “Fixed” and “Changed” sections of the changelog. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utilities.js (1)
88-93: 🧹 Nitpick | 🔵 TrivialStale JSDoc — description and return type don't match actual usage.
The function was renamed from
resetAndGetOrientationtoresetOrientation, but the JSDoc still reads "Get orientation value from given array buffer" and declares@returns {number}. Two inaccuracies:
- The primary purpose is now the mutation side-effect (resetting the EXIF orientation tag to
1in place); the return value is discarded by every caller.orientationisundefinedwhen no orientation tag is found without an error, so{number}is incorrect.📝 Suggested JSDoc update
/** - * Get orientation value from given array buffer. + * Reset the EXIF orientation tag in a JPEG array buffer to 1 (default). + * Mutates the buffer in place. Returns the original orientation value + * (before resetting), or `undefined` if no orientation tag was found. * `@param` {ArrayBuffer} arrayBuffer - The array buffer to read. - * `@returns` {number} The read orientation value. + * `@returns` {number|undefined} The original orientation value, or `undefined` if not present. */ export function resetOrientation(arrayBuffer) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utilities.js` around lines 88 - 93, The JSDoc for resetOrientation is stale: update the description to state that the function resets the EXIF orientation tag to 1 in-place (mutation side-effect) rather than "getting" it, and change the `@returns` annotation to reflect the actual possible values (e.g., `@returns` {number|undefined} previous orientation value or undefined if no tag was found) or remove the return tag if you decide the function should not return anything; ensure the JSDoc mentions the mutation behavior and references the resetOrientation function so callers understand the side-effect.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/tests.yml:
- Line 7: The workflow_dispatch event is added but the full-browser-matrix job
(job names: test-macos and the cross-browser matrix) is gated by the hard-coded
condition "github.event_name == 'push' && github.ref == 'refs/heads/main'", so
manual runs skip cross-browser validation; update the condition used by the
test-macos/cross-browser job to also allow workflow_dispatch (e.g., include
"github.event_name == 'workflow_dispatch'" OR combine into a predicate that
permits either push to main or a manual dispatch) so that the
full-browser-matrix runs for manual workflow_dispatch triggers as well.
In `@src/index.js`:
- Around line 432-453: The catch branch for the strictFallback EXIF-strip flow
silently falls back to returning the original file; update the catch handler
(the promise chain that starts with file.arrayBuffer() used alongside stripExif,
uint8ArrayToBlob and options.success) to emit a diagnostic (e.g., console.warn)
indicating EXIF stripping failed and include contextual details (file.name,
error message and the retainExif=false/strictFallback state) before setting
this.result and calling options.success so callers can detect the fallback; keep
the existing behavior but add the warning and ensure you reference the same
symbols (file.name, file.lastModified, this.result, options.success) when adding
the log.
- Around line 319-340: The async path that reads blob data for the
retainExif+insertExif flow doesn't check this.aborted before proceeding, so if
abort() is called while blob.arrayBuffer() or FileReader is in flight the next
callback can still call done() and trigger success after fail; update the code
paths where blob is read (the branch using blob.arrayBuffer() and the FileReader
handlers — reader.onload/next) to check if (this.aborted) return (or skip
calling next/done) before invoking next()/done(), mirroring the guard used in
the stripExif branch and ensuring abort() reliably prevents any subsequent calls
to this.fail/this.success.
In `@test/specs/output.spec.js`:
- Around line 4-6: Replace the non-standard comment annotation "// @@ Revisit
when WebKit adds WebP encoding support" with a conventional TODO-style comment;
update the line near the isWebKit detection (the const isWebKit = ...
declaration and its preceding comment) to use "// TODO: Revisit when WebKit adds
WebP encoding support" (or remove the @@ prefix) so IDEs, linters, and
grep-tools recognize it.
---
Outside diff comments:
In `@src/utilities.js`:
- Around line 88-93: The JSDoc for resetOrientation is stale: update the
description to state that the function resets the EXIF orientation tag to 1
in-place (mutation side-effect) rather than "getting" it, and change the
`@returns` annotation to reflect the actual possible values (e.g., `@returns`
{number|undefined} previous orientation value or undefined if no tag was found)
or remove the return tag if you decide the function should not return anything;
ensure the JSDoc mentions the mutation behavior and references the
resetOrientation function so callers understand the side-effect.
Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
Added a check to halt processing if the operation was aborted. Improved error logging with detailed messages when EXIF stripping fails, allowing the original file to be returned intact. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/tests.yml:
- Around line 53-54: The workflow restricts the test-macos job (which runs npm
run test:full) to main-branch pushes via the condition if: github.event_name ==
'push' && github.ref == 'refs/heads/main', causing PRs to run only the
single-browser npm test; either remove or relax that condition to include
pull_request events (e.g., allow github.event_name == 'pull_request' or combine
push|pull_request) so the full browser matrix runs on PRs, or alternatively
change the job to run a reduced matrix for pull_request while keeping full
matrix on main, or document this trade-off if you intentionally want PRs to skip
npm run test:full; update the test-macos job and its if condition accordingly.
In `@src/index.js`:
- Around line 433-461: The current strictFallback branch checks file.arrayBuffer
and only logs errors inside the Promise.catch, but if file.arrayBuffer is falsy
the code falls through and returns the original JPEG with EXIF intact; update
the branch that tests strictFallback && file.type === 'image/jpeg' to detect the
absence of file.arrayBuffer, emit the same console.warn used in the .catch()
path, and implement a FileReader-based fallback (similar to the canvasFallback
approach): use FileReader.readAsArrayBuffer to load the blob, call stripExif on
the ArrayBuffer, convert with uint8ArrayToBlob, preserve name/lastModified, set
this.result and invoke options.success.call(this, stripped) while respecting
this.aborted checks; keep the existing Promise-based path (file.arrayBuffer())
unchanged and mirror its error handling/messages for consistency.
- Around line 147-155: The retainExif FileReader onload handler (in the
reader.onload block that calls resetOrientation, getExif, sets this.url and
this.load) needs a guard for this.aborted so the handler bails when an abort
raced with the read; update the onload callback to check if (this.aborted) {
this.fail(); return; } before creating the object URL or calling this.load,
ensuring the same abort-handling behavior as canvasFallback and allowing fail()
to run when a close/abort race occurs.
Added abort checks to prevent further execution when processing is terminated. Improved fallback handling for EXIF data stripping by introducing FileReader usage when array buffer support is unavailable, ensuring consistent behavior across environments. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
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/index.js (1)
322-374: 🧹 Nitpick | 🔵 TrivialDuplicated FileReader +
blob.arrayBuffer()fallback pattern — three near-identical blocks.The new
retainExifFileReader fallback (lines 327–342) is functionally identical to the existingstripExifFileReader fallback (lines 358–373), and a third copy lives indone()'sstrictFallbackpath (lines 463–499). The only difference between each block is thenextfunction body. Extracting a shared helper would eliminate the duplication and reduce drift risk when any one copy is changed.♻️ Sketch of a shared helper
+ /** + * Reads `blob` as an ArrayBuffer and calls `process(arrayBuffer)`. + * Falls back to FileReader when `Blob.prototype.arrayBuffer` is unavailable. + */ + _readBlobAsArrayBuffer(blob, process, onError) { + if (blob.arrayBuffer) { + blob.arrayBuffer().then(process).catch(() => { + if (this.aborted) return; + onError(); + }); + } else { + const reader = new FileReader(); + this.reader = reader; + reader.onload = ({ target }) => { process(target.result); }; + reader.onabort = () => { + this.fail(new Error('Aborted to read the image with FileReader.')); + }; + reader.onerror = () => { + if (this.aborted) return; + onError(); + }; + reader.onloadend = () => { this.reader = null; }; + reader.readAsArrayBuffer(blob); + } + }Each call-site then reduces to
this._readBlobAsArrayBuffer(blob, next, () => this.fail(...)).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.js` around lines 322 - 374, Extract the repeated Blob -> ArrayBuffer logic into a single instance method (e.g. _readBlobAsArrayBuffer(blob, onSuccess, onError)) and replace the three duplicated blocks with calls to it; the helper should check blob.arrayBuffer() and use its Promise if present, otherwise create a FileReader, assign this.reader, set onload to call onSuccess(target.result), set onabort/onerror to call onError, clear this.reader in onloadend and call readAsArrayBuffer(blob). Update the call-sites that currently create a local next (the canvas path, the retainExif path that calls uint8ArrayToBlob(stripExif(...)), and the done() strictFallback path) to pass their next handler as onSuccess and a small lambda that calls this.fail(new Error(...)) as onError.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/index.js`:
- Around line 317-325: The blob.arrayBuffer().catch() handlers in the retainExif
path call this.fail() without checking this.aborted, which can double-invoke
options.error if abort() already triggered this.fail(); update the catch
callbacks (the one after blob.arrayBuffer().then(next).catch(...) in the
retainExif flow) to first check if (this.aborted) return; before calling
this.fail(), and apply the identical guard to the equivalent catch in the
stripExif path (the blob.arrayBuffer() catch there) so both paths match the
strictFallback behavior that prevents double error invocations.
---
Outside diff comments:
In `@src/index.js`:
- Around line 322-374: Extract the repeated Blob -> ArrayBuffer logic into a
single instance method (e.g. _readBlobAsArrayBuffer(blob, onSuccess, onError))
and replace the three duplicated blocks with calls to it; the helper should
check blob.arrayBuffer() and use its Promise if present, otherwise create a
FileReader, assign this.reader, set onload to call onSuccess(target.result), set
onabort/onerror to call onError, clear this.reader in onloadend and call
readAsArrayBuffer(blob). Update the call-sites that currently create a local
next (the canvas path, the retainExif path that calls
uint8ArrayToBlob(stripExif(...)), and the done() strictFallback path) to pass
their next handler as onSuccess and a small lambda that calls this.fail(new
Error(...)) as onError.
Included checks to stop execution if the operation was aborted during Blob.arrayBuffer() reading. This prevents unnecessary error handling and ensures that aborted operations do not trigger failure callbacks. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
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.
Summary by CodeRabbit
Breaking Changes
New Features
Bug Fixes
Documentation
Tests
Types