File image resize#4160
Conversation
|
Warning Review limit reached
Next review available in: 51 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (15)
📝 WalkthroughWalkthroughAdds client-side image resizing to ChangesImage resize feature
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-4160/ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/components/file/examples/file-resize-image.tsx`:
- Around line 146-152: The alt text in renderPreview is redundant because it
repeats the “image” wording that screen readers already announce for the img
element. Update the alt text in the FileResizeImage preview rendering to
describe the content of the preview without using the word “image,” keeping the
meaning clear and accessible.
- Around line 169-179: The getters in file-resize-image, getSelectedFit and
getSelectedType, are typed as returning Option but they use Array.prototype.find
and can actually return undefined. Update both return types to include undefined
or otherwise handle the missing-match case explicitly so the type signature
matches the real behavior and the mismatch cannot be hidden from the checker.
In `@src/components/file/examples/file-resize-mixed.tsx`:
- Around line 65-78: The renderCallout() helper in file-resize-mixed returns
multiple top-level JSX nodes as a raw array, which violates the JSX array/Host
guideline. Replace the array literal with a single wrapper such as a Fragment or
plain container element around the hr and limel-callout elements, while keeping
the existing notResized guard and content unchanged.
In `@src/util/image-resize.ts`:
- Around line 189-227: The dimension fallback logic in resolveTargetSize
currently uses plain truthy checks, so negative values can pass through even
though the docstring says non-positive requests should be treated as missing.
Update the width/height guards in resolveTargetSize to explicitly require
positive numbers before using them, and keep the existing aspect-ratio
derivation behavior for single-dimension requests. Make sure any callers like
computeRects/createCanvas only receive validated positive dimensions, with
invalid inputs falling back to the source size.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 58708975-42c9-4a60-abf4-32afdd194544
⛔ Files ignored due to path filters (1)
etc/lime-elements.api.mdis excluded by!etc/lime-elements.api.md
📒 Files selected for processing (14)
src/components/file/examples/file-resize-image.scsssrc/components/file/examples/file-resize-image.tsxsrc/components/file/examples/file-resize-mixed.tsxsrc/components/file/file.spec.tsxsrc/components/file/file.tsxsrc/translations/da.tssrc/translations/de.tssrc/translations/en.tssrc/translations/fi.tssrc/translations/fr.tssrc/translations/nl.tssrc/translations/no.tssrc/translations/sv.tssrc/util/image-resize.ts
Befkadu1
left a comment
Befkadu1
left a comment
There was a problem hiding this comment.
Review notes
A few findings from reviewing the resize integration. The two on file.tsx are worth a look; the rest are minor. Inline comments below.
One finding that isn't on a changed line, so noting it here: renderDragAndDropTip() (src/components/file/file.tsx:230) still guards on this.value, while the rest of the render path moved to displayedFile. In the window after a resize succeeds (resizingFile set, isBusy false) but before the consumer commits value, the "Drag & drop your file here" tip can render alongside the resized chip. Switching that guard to this.displayedFile keeps it consistent. Only reproduces when the consumer commits value asynchronously.
Befkadu1
force-pushed
the
file-image-resize
branch
2 times, most recently
from
e627555 to
84c7c78
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/components/file/file.tsx (1)
251-304: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStale re-emit still possible when removing the chip during initial resize.
The
pendingtoken guard (line 295-297) only gets invalidated via@Watch('value'), which fires only whenvalueactually changes. In the common flow wherevaluestarts asundefinedand the user removes the loading chip mid-resize,handleChipSetChangeemitschangewithundefined— but sincevaluewas alreadyundefined, the watcher never fires,resizingFilestays equal topending, and the completed resize still re-shows/re-emits the file the user just removed.This is the same underlying race flagged in a previous review; the token check narrows but doesn't close it for the "no prior value" case, since the chip-set stays interactive during resize (
disabled={this.disabled}only).🐛 Proposed fix: clear the token directly on removal
private handleChipSetChange = (event: CustomEvent) => { event.stopPropagation(); const file = event.detail.length === 0 ? event.detail[0] : null; if (!file) { + this.resizingFile = undefined; this.change.emit(file); } };🤖 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/components/file/file.tsx` around lines 251 - 304, The stale re-emit race in handleNewFiles persists when a loading chip is removed before value changes, because the resizing token is only cleared by the value watcher. Update the removal path for the interactive chip set (handleChipSetChange / the code that emits change(undefined)) so it clears resizingFile immediately when a chip is removed, ensuring the pending token created in handleNewFiles is invalidated even when value was previously undefined. Keep the existing pending/out guard in handleNewFiles, but make token invalidation happen directly on removal.
🤖 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.
Duplicate comments:
In `@src/components/file/file.tsx`:
- Around line 251-304: The stale re-emit race in handleNewFiles persists when a
loading chip is removed before value changes, because the resizing token is only
cleared by the value watcher. Update the removal path for the interactive chip
set (handleChipSetChange / the code that emits change(undefined)) so it clears
resizingFile immediately when a chip is removed, ensuring the pending token
created in handleNewFiles is invalidated even when value was previously
undefined. Keep the existing pending/out guard in handleNewFiles, but make token
invalidation happen directly on removal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9443c898-645d-4eea-aea2-17ee2e25ea1d
⛔ Files ignored due to path filters (1)
etc/lime-elements.api.mdis excluded by!etc/lime-elements.api.md
📒 Files selected for processing (13)
src/components/file/examples/file-resize-image.scsssrc/components/file/examples/file-resize-image.tsxsrc/components/file/examples/file-resize-mixed.tsxsrc/components/file/file.tsxsrc/translations/da.tssrc/translations/de.tssrc/translations/en.tssrc/translations/fi.tssrc/translations/fr.tssrc/translations/nl.tssrc/translations/no.tssrc/translations/sv.tssrc/util/image-resize.ts
Befkadu1
force-pushed
the
file-image-resize
branch
2 times, most recently
from
a636ca6 to
d8cad81
Compare
Befkadu1
left a comment
Befkadu1
left a comment
There was a problem hiding this comment.
PR Review: File image resize
Intent: Add optional client-side image resizing to limel-file (downscale + re-encode before change fires), with an "Optimizing…" progress state and graceful fallback for non-decodable files. Fixes lime-webclient#7902.
Scope: +599/−32 across 14 files (1 core file, 1 util, 2 examples, 8 translations, 1 API report).
Findings
Backward compatibility: ✅ — no breaks introduced.
ResizeOptions.width/heightgo required → optional (image-resize.ts:113-124). Widening an input object is compatible for existing callers, and the type is@beta.resizeImage(file, options)signature is unchanged.- New
resizeImageprop is additive/optional;file.optimizingadded to all 8 locales;etc/lime-elements.api.mdregenerated to match. - One behavior change beyond the title's scope:
clearAllButton={false}(file.tsx:365) removes the clear-all button for everylimel-fileconsumer, not just resize users. It's a reasonable call for a single-file picker (the chip's own × remains), but confirm you intend it to land in this PR rather than a standalone one.
Code quality:
file.tsx:180-182: the@Watchcomment says it keeps "the same chip mounted across the hand-off (see file.spec.tsx) so only its badge changes instead of the field flashing empty." Butfile.spec.tsxisn't in the PR (dropped during rebase), and the mount-stability change it documented is also gone —renderChipsetstill returns the bare chip-set whenvalueis set (file.tsx:378-380), so the wrapper→bare swap on commit still remounts and can flash. The comment describes behavior the code no longer implements. Worth checking: restore the wrapper fix + spec, or correct the comment.- No automated coverage for the new async logic (resize, the token-guard bail at
file.tsx:300, removal-cancels-resize atfile.tsx:402). This logic had real bugs during development; it's exactly what a unit test should pin. - Minor:
updateNumericOption'skeyunion includes'quality', which never flows through it (examples/file-resize-image.tsx).
Architecture: ✅ — resize isolated in a dependency-free util; component holds only the transient resizingFile state + displayedFile derivation. Clean separation, no new cross-cutting mechanisms.
Security: ✅ — purely client-side canvas work; no injection surface, no innerHTML, no secrets. Object URLs are revoked in the example (componentWillUnload + on change). The re-encode caveat (original bytes destroyed → bad for checksums/scanning) is explicitly documented.
Observability: ✅ (mostly N/A for a UI lib) — the catch {} at file.tsx:293 swallows resize failures silently, but that's the documented "best-effort → emit original" contract, and the examples show how consumers detect a skipped resize. image-resize.ts has a dev-only gated console.debug on the decode fallback — acceptable.
Performance:
- Resize runs on the main thread (
OffscreenCanvaswhen available but no worker), so a large image briefly blocks the UI; mitigated by the "Optimizing…" badge and documented perf tips. - Cancel (
file.tsx:402) drops the result but can't abort the in-flightcreateImageBitmap/encode, so a cancelled large resize still burns CPU to completion. Bounded (dropzone is disabled during resize), so no unbounded concurrency.
Merge readiness: CHANGES REQUESTED
No correctness, security, or compatibility regressions — functionally the branch is strictly better than main and the resize/cancel logic is now correct. This isn't blocked on a defect. The two things I'd want resolved first are quality, not safety: (1) the feature's unit tests (file.spec.tsx) were lost in the rebase, leaving the newly-fixed async logic uncovered; and (2) the @Watch comment references that missing file and claims a no-flash guarantee the code no longer delivers (the mount-stability fix was dropped too). Restore both — or drop the comment's claim and add coverage — and this is READY. If you'd rather ship now and follow up, none of the above will break consumers.
🤖 Generated with Claude Code
`ResizeOptions.width` and `height` are now optional. A missing dimension is derived from the given one to preserve the source aspect ratio, and omitting both keeps the source dimensions so the image is only re-encoded. Both were previously required, forcing every caller to pick an exact target box. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Set `resizeImage` with a `ResizeOptions` config to downscale and re-encode a selected image on the user's device before the `change` event fires, reducing upload size and normalising dimensions and format. Only decodable raster images are resized; non-images, SVGs and images the browser cannot decode are emitted unchanged, and any resize failure falls back to the original file. Adds examples for an image-restricted picker and for a general-purpose picker. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Client-side resize is asynchronous, so between selecting an image and the `change` event the field previously looked idle. Show the selected file immediately as a chip with a loading indicator and a localised "Optimizing…" status badge, and keep that same chip mounted as the committed `value` arrives so only the badge changes rather than the field flashing empty. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`limel-file` selects a single file, so there is at most one chip, which
already has its own remove button. The chip set's "clear all" button was
therefore redundant; disable it via `clearAllButton={false}`.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add spec and e2e coverage for the `resizeImage` feature on `limel-file`. The spec (mock-doc) covers what needs no decoder: the clear-all button being disabled for the single-file picker, and that non-images, SVGs, and the no-`resizeImage` case are emitted untouched. The e2e (browser) covers what needs a real canvas: the resize round-trip (a selected PNG is downscaled and re-encoded to JPEG) and cancelling a resize mid-flight (removing the chip must not re-emit the stale result). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Befkadu1
force-pushed
the
file-image-resize
branch
from
e961156 to
bfd7725
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/file/file.tsx (1)
378-389: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse
displayedFilehere, notvalue. During resize,valueis still empty butdisplayedFileis set, so<limel-file-input>stays clickable even though the dropzone is disabled. Swapping the guard keeps file picking blocked until the resize hand-off finishes.🤖 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/components/file/file.tsx` around lines 378 - 389, The guard in the file rendering branch is using value instead of displayedFile, which leaves limel-file-input clickable during resize hand-off. Update the condition in the file component’s render logic to check displayedFile so the chipset-only path is used while the resize state is active, keeping the dropzone disabled until the hand-off completes.
🤖 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.
Outside diff comments:
In `@src/components/file/file.tsx`:
- Around line 378-389: The guard in the file rendering branch is using value
instead of displayedFile, which leaves limel-file-input clickable during resize
hand-off. Update the condition in the file component’s render logic to check
displayedFile so the chipset-only path is used while the resize state is active,
keeping the dropzone disabled until the hand-off completes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5095a72c-f925-4c6b-866d-e3cbd454b12d
📒 Files selected for processing (11)
src/components/file/file.e2e.tsxsrc/components/file/file.spec.tsxsrc/components/file/file.tsxsrc/translations/da.tssrc/translations/de.tssrc/translations/en.tssrc/translations/fi.tssrc/translations/fr.tssrc/translations/nl.tssrc/translations/no.tssrc/translations/sv.ts
|
🎉 This PR is included in version 39.42.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
3 participants
fix https://github.com/Lundalogik/lime-webclient/issues/7902
Summary by CodeRabbit
Review:
Browsers tested:
(Check any that applies, it's ok to leave boxes unchecked if testing something didn't seem relevant.)
Windows:
Linux:
macOS:
Mobile: