Repository navigation
Refuse images over the browser canvas size limits on import - #146
Open
officialasishkumar wants to merge 1 commit into
Open
officialasishkumar wants to merge 1 commit into
officialasishkumar wants to merge 1 commit into
Conversation
Codex Review Summary: ❌ FailedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Importing an image larger than a browser canvas can hold, for example a 17000x17000 PNG, appeared to work: the size badge showed the new dimensions, but the 2D preview stayed blank and the app stayed unresponsive for minutes. CanvasPreview copies the image into canvases at full size, and browsers silently refuse canvases over 268,435,456 pixels (16384x16384) in Chromium, the WebView2 desktop app and WebKit, or with a side over 32,767 pixels in Firefox. The refused canvases stay blank instead of throwing, so the rest of the pipeline kept working against unusable canvases (vycdev#125). useImageImport now reads the decoded size before closing the image and refuses anything outside both limits, through the same path as unreadable files: the object URL is revoked, nothing is pushed to image history, the swatches are left alone, and the dismissible import notice explains the problem. The check lives in the main import rather than validateImageFile because the Stack Matrix photo picker also uses that helper and draws photos at no more than 1200 px, so very large photos still work there. - src/lib/imageFile.ts: add MAX_IMAGE_SIDE, MAX_SQUARE_IMAGE_SIDE, MAX_IMAGE_PIXELS and fitsImageCanvasLimits, with the per-engine limits documented. - useImageImport: new 'tooLarge' error kind carrying the file name, the image size and whether the current image was kept, so the notice follows language changes like the existing kinds. - App: build the notice text in a small helper instead of a nested ternary. Every translation key stays a literal for check:i18n. - Two new strings in all 12 locales name the image size and the 16384x16384 limit, with and without the "current image was kept" sentence. - Loading Images docs, in all 12 languages, describe the limits. - Unreleased changelog entry under Fixed. Tests: unit coverage of the limits at and just past both boundaries, and a @smoke e2e test that imports a real 32768x8 PNG, generated in the test, and checks the notice text, the kept image, that history and the "no image" wording are unchanged, and that a 32767x8 PNG still imports. Fixes vycdev#125
officialasishkumar
force-pushed
the
fix/issue-125-oversized-image-import
branch
from
November 11, 2026 16:32
0d7fe5e to
69ebcfd
Compare
Author
|
The e2e jobs here failed in the "Install Playwright system dependencies" step, before any test ran: the runner couldn't resolve |
This branch has not been deployed
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
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.
Importing an image larger than a browser canvas can hold, such as the 17000×17000 PNG from #125, looked like it worked: the size badge showed the new dimensions, but the 2D preview stayed blank and the app stayed sluggish with no message.
CanvasPreviewcopies the image into canvases at full size. Browsers refuse oversized canvases without throwing, so the canvases just stay blank:Change
The main image import now refuses an image outside either limit, before it reaches image history. It goes through the same path as unreadable files from #107: the object URL is revoked, history and swatches are left alone, and the dismissible import notice explains why:
src/lib/imageFile.tsaddsfitsImageCanvasLimitsand the limit constants, with each engine's limit documented.useImageImportreads the decoded size before closing the image (a closedImageBitmapreports 0×0) and reports a newtooLargeerror kind. Like the existing kinds, it stores the name, size, and whether an image was kept, not a translated string, so the notice follows language changes.useImageImportrather thanvalidateImageFile. The Stack Matrix photo picker also usesvalidateImageFile, but it draws photos at 1200 px at most, so very large photos keep working there.App.tsxbuilds the notice text in a small helper instead of a three-level ternary. Each translation key stays a literal socheck:i18nstill checks it.This PR refuses the image rather than offering to downscale it. The issue lists either as acceptable, and downscaling only to the canvas limit would still leave a very slow image (the issue measured about 19 s to recover at 12000×12000). If you'd prefer a downscale option in the notice, I can add it as a follow-up.
Fixes #125
Tests
tests/imageFile.test.ts:fitsImageCanvasLimitsat and just past both limits.tests/e2e/image-import-error.spec.ts(@smoke): the test generates a real 32768×8 PNG and imports it over a loaded image. It checks the notice text, that the current image is kept, that no history entry is added, and the "no image" wording after Remove image. It also checks that a 32767×8 PNG still imports. The test fails ondevelop, where no notice appears, and passes with this change.page.evaluateround trip took 15 ms right afterwards.npm run lint,npx tsc -b,npm run check:i18n, Prettier on the changed source and test files, and theimage-import-errorandimage-reimporte2e specs (10 passed) all pass.The privacy and terms e2e jobs currently fail on
developbecause of #143, which is fixed separately.