feat: support TIFF images in DOCX rendering#2193
feat: support TIFF images in DOCX rendering#2193gpardhivvarma wants to merge 11 commits intosuperdoc-dev:mainfrom
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6335817b6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
packages/super-editor/src/core/super-converter/v3/handlers/wp/helpers/tiff-converter.js
Show resolved
Hide resolved
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f0832d14d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
packages/super-editor/src/core/super-converter/v3/handlers/wp/helpers/tiff-converter.js
Outdated
Show resolved
Hide resolved
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c061d636b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
packages/super-editor/src/core/super-converter/v3/handlers/wp/helpers/tiff-converter.js
Outdated
Show resolved
Hide resolved
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
nice work - clean structure, follows the metafile converter pattern well, DoS guard is in the right place.
added a few comments
...es/super-editor/src/core/super-converter/v3/handlers/wp/helpers/encode-image-node-helpers.js
Outdated
Show resolved
Hide resolved
packages/super-editor/src/core/super-converter/v3/handlers/wp/helpers/tiff-converter.test.js
Show resolved
Hide resolved
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
@gpardhivvarma all three comments from last round are resolved, and the overall approach looks good. left two small inline suggestions — nothing blocking, just minor cleanup.
...es/super-editor/src/core/super-converter/v3/handlers/wp/helpers/encode-image-node-helpers.js
Show resolved
Hide resolved
packages/super-editor/src/core/super-converter/v3/handlers/wp/helpers/tiff-converter.js
Outdated
Show resolved
Hide resolved
|
@caio-pizzol made the requested additions please take a look at them |
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
@gpardhivvarma all the feedback from the previous rounds is addressed — thanks for working through it. the code looks clean, left one inline suggestion about an integration test you could add following an existing pattern. not blocking. we'll handle uploading a TIFF doc for visual regression testing on our end since that requires R2 credentials.
Integration test suggestion
there's an existing pattern in tests/behavior/tests/importing/load-doc-with-pict.spec.ts that loads a .docx, waits for stable, and asserts the editor is functional — a similar test for a TIFF-containing .docx would verify the full pipeline end-to-end (DocxZipper → converter → convertTiffToPng → rendered img). would you be able to create a simple .docx with a TIFF image and follow the same pattern?
Sure would be happy to help, will update once done |
|
@caio-pizzol please take a look |
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
@gpardhivvarma solid work on this — the converter follows the existing metafile-converter pattern nicely, the pixel limit guard is well-placed, and the new behavior test with a real fixture is a good addition.
one minor suggestion: the imageTypes/imageExts and mimeTypeForExt mappings in DocxZipper.js are duplicated across two methods — could be extracted to module-level constants. not blocking, just a cleanup for later.
lgtm, approving.
|
I noticed maybe worth refactoring? |
Yeah sounds good will work on the refactor and update the PR |
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
@gpardhivvarma the utif2 → tiff swap is clean and all prior feedback is addressed.
two new things from this round: decode() loads all pixel data up front, so the size guard runs too late — left a suggestion using { ignoreImageData: true } to check dimensions first. also, 16-bit TIFFs would get wrong colors because of a type mismatch — left a snippet for that too.
on tests: three small gaps — TIFF failure fallback in handleImageNode, the greyscale/alpha branches in toRGBA, and .tif MIME mapping in DocxZipper. not blocking, just nice to have.
packages/super-editor/src/core/super-converter/v3/handlers/wp/helpers/tiff-converter.js
Outdated
Show resolved
Hide resolved
packages/super-editor/src/core/super-converter/v3/handlers/wp/helpers/tiff-converter.js
Show resolved
Hide resolved
gpardhivvarma
force-pushed
the
feat/tiff-image-support
branch
from
643836a to
ee33535
Compare
TIFF images in DOCX files rendered as broken icons because browsers cannot natively display image/tiff. Convert TIFF to PNG at import time using utif2, following the existing EMF/WMF → SVG conversion pattern. Closes superdoc-dev#2064
Reject TIFF images exceeding 100M pixels before allocating RGBA buffers or canvas, preventing a malicious TIFF with extreme dimensions from freezing or crashing the tab during import.
Move MAX_PIXEL_COUNT check before UTIF.decodeImage/toRGBA8 so oversized TIFFs are rejected before allocating the RGBA buffer. Map .tif extension to image/tiff in Content_Types.xml generation to avoid emitting the invalid MIME type image/tif.
UTIF.decode populates raw tag entries (t256/t257) but .width/.height are only set after decodeImage. Read from raw tags so the pixel limit guard works before the expensive decode step without rejecting valid files.
- Use mimeTypeForExt mapping for .tif data URIs (image/tiff not image/tif) - Remove unused size arg from convertTiffToPng call - Add happy-path test asserting valid TIFF produces PNG data URI
- Remove unused Uint8Array/ArrayBufferView branches (only strings are passed) - Add handleImageNode test verifying convertTiffToPng is called for .tif files
Adds a Playwright test that loads a minimal DOCX containing a TIFF image and verifies the full pipeline: DocxZipper → convertTiffToPng → rendered PNG.
… constants Replace unmaintained utif2 with actively maintained image-js/tiff for TIFF decoding. Extract duplicated IMAGE_EXTS and MIME_TYPE_FOR_EXT mappings in DocxZipper.js to module-level constants.
Use decode(buffer, { ignoreImageData: true }) to check dimensions
before allocating pixel data, preventing DoS from small compressed
TIFFs with huge dimensions. Normalize Uint16Array and Float32Array
pixel data to 8-bit for canvas compatibility.
gpardhivvarma
force-pushed
the
feat/tiff-image-support
branch
from
ee33535 to
10d064e
Compare
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
@gpardhivvarma the ignoreImageData guard and bit-depth normalization both look correct — nice work.
main thing: the tif→tiff MIME fix has no test yet, and it's the one place a regression could silently break export. left a few inline comments, nothing blocking.
packages/super-editor/src/core/super-converter/v3/handlers/wp/helpers/tiff-converter.js
Outdated
Show resolved
Hide resolved
...per-editor/src/core/super-converter/v3/handlers/wp/helpers/encode-image-node-helpers.test.js
Show resolved
Hide resolved
packages/super-editor/src/core/super-converter/v3/handlers/wp/helpers/tiff-converter.test.js
Show resolved
Hide resolved
…red dataUriToArrayBuffer helper Address remaining PR review feedback: add tests for .tif → image/tiff MIME mapping (import data URI and export Content_Types), TIFF conversion failure fallback alt text, greyscale/grey+alpha/Uint16/Float32 toRGBA branches, and extract duplicate data-URI-stripping logic from metafile-converter and tiff-converter into shared dataUriToArrayBuffer in helpers.js.
|
@caio-pizzol @harbournick Please mind taking a look. |
|
Closing in favor of a new PR from main repo branch — all original commits preserved. Thank you @gpardhivvarma for the implementation! |
2 participants
Summary
utif2, following the existing EMF/WMF → SVG conversion patterntifto DocxZipper image extension sets so.tiffiles are properly extracted as data URIsoriginalSrc/originalExtensionattributesCloses #2064
Test plan
isTiffExtension(tiff/tif/TIFF/other/null/undefined)convertTiffToPng(invalid data, null, empty, non-TIFF base64)