render-helper: decode entities with entities instead of he - #1606
Conversation
Code Review by Qodo
1.
|
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 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 |
PR Summary by Qodorender-helper: replace he.decode with entities.decodeHTML via safe decodeEntities
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
Greptile SummaryThis PR replaces
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| packages/render-helper/src/helper.ts | Introduces the shared decodeEntities helper with leading-zero normalization, overlong-reference replacement, and guarded decoding. |
| packages/render-helper/src/decode-entities.spec.ts | Covers ordinary, padded, overlong, and semicolonless references across summary and image-processing paths. |
| packages/render-helper/src/catch-post-image.ts | Replaces direct he.decode calls with the shared decoder for metadata and body image URLs. |
| packages/render-helper/src/post-body-summary.ts | Uses the shared decoder for final post-summary entity decoding. |
| packages/render-helper/package.json | Updates the package version and replaces he with entities. |
| packages/render-helper/tsup.config.ts | Externalizes entities in place of the removed he dependency. |
Reviews (3): Last reviewed commit: "chore: apply changeset versioning for PR..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: e01cac92a9
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * produce for an out-of-range code point, and anything else the decoder | ||
| * rejects is returned as-is. | ||
| */ | ||
| const OVERLONG_NUMERIC_REF = /&#(?:x[0-9a-f]{256,}|[0-9]{309,});?/gi |
There was a problem hiding this comment.
This length-only check also replaces valid references whose digits are mostly leading zeros. For example, &# followed by 309 zeros and 65; has more than 309 digits but evaluates to code point 65, so both the previous he.decode implementation and decodeHTML produce A; this pre-pass instead forces it to U+FFFD. Strip leading zeros or determine whether the parsed value actually overflows before replacing the reference.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right, and taken. Leading zeros are now stripped before the overflow check (decimal and hex patterns kept separate so a decimal reference followed by a hex letter is untouched), so &# + 400 zeros + 65; decodes to A as it did with he; only a reference that is still overlong after that becomes U+FFFD. A differential run of he.decode against the new helper over 15,190 inputs, including zero-padded and mixed cases, shows no mismatches, and the spec covers the padded forms.
|
The red |
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
The package imported he for he.decode in three places while entities was already in the browser bundle through htmlparser2 and dom-serializer. Two HTML entity decoders cost about 62 KB gzipped on every web route; dropping he saves about 30 KB of that. A decodeEntities helper wraps entities.decodeHTML for the three call sites. A differential run over 14,091 inputs (every named reference he knows, with and without the semicolon and with trailing text, numeric and hex references across the BMP and astral planes, malformed and legacy forms) produced identical output from both libraries, with one class of exception found in review: decodeHTML throws a RangeError on a numeric reference with 309+ decimal or 256+ hex digits, where he returned U+FFFD. Bodies and json_metadata are user-authored, so the helper replaces such references with U+FFFD first and returns the input unchanged if the decoder still rejects it. Regression specs cover decodeImageSrc, catchPostImage (metadata string, metadata array, body image) and getPostBodySummary with that payload. Closes #1596
e01cac9 to
d7fbd34
Compare
1 participant
@ecency/render-helperimportedheforhe.decodein three places (helper.ts, post-body-summary.ts, catch-post-image.ts) whileentitieswas already in the browser bundle through htmlparser2 and dom-serializer. Two HTML entity decoders cost about 62 KB gzipped on every web route; droppinghesaves about 30 KB of that.A
decodeEntitieshelper wrapsentities.decodeHTMLfor the three call sites and thehe/@types/hedependencies are removed. One behavioural difference was found in review:decodeHTMLthrows aRangeErroron a numeric reference whose digit string overflows a double (309+ decimal or 256+ hex digits, leading zeros included), wherehereturned U+FFFD. Bodies andjson_metadataare user-authored, so the helper strips leading zeros first (Aand&#+ 400 zeros +65;are stillA), replaces a reference that is still overlong with U+FFFD (what the HTML spec andheproduce for an out-of-range code point), and returns the input unchanged if the decoder still rejects it. A differential run ofhe.decodeagainst the helper over 15,190 inputs (every named referenceheknows with and without the semicolon and with trailing text, numeric and hex references across the BMP and astral planes, zero-padded, malformed and legacy forms) produced identical output.dist/is not rebuilt here; the tracked build output still importshe, which this PR removes from the lockfile, so the release flow's dist rebuild needs to land with the merge rather than after it (the redtestscheck is that tracked dist).Test plan
decode-entities.spec.ts: parity on ordinary references, zero-padded references, overlong decimal / hex / no-semicolon references decode to U+FFFD without throwing, and the payload is exercised throughdecodeImageSrc,catchPostImage(metadata string, metadata array, body image) andgetPostBodySummaryat the description length and unbounded.tsc --noEmitandeslintclean; web specs that consume the rebuilt dist (slim-entry, entry thumbnails, landing page) pass.Closes #1596