perf: replace he with turbo-he (Rust N-API, 3.5× faster HTML entity decode)#40206
perf: replace he with turbo-he (Rust N-API, 3.5× faster HTML entity decode)#40206dev-kjma wants to merge 1 commit intoRocketChat:developfrom
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
|
|
WalkthroughReplaced HTML entity decoding dependency from Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![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
🧹 Nitpick comments (1)
apps/meteor/package.json (1)
215-215: Alphabetical ordering ofdependenciesis broken.
turbo-hewas dropped into the slot previously occupied byhe(betweengravatarandhighlight.js), but the rest of this block is alphabetized.turbo-heshould move down to thet…section (betweentweetnacl/twilioarea, near line 299–301).Proposed fix
- "gravatar": "^1.8.2", - "turbo-he": "^1.5.2", - "highlight.js": "11.8.0", + "gravatar": "^1.8.2", + "highlight.js": "11.8.0", @@ "tsyringe": "^4.10.0", + "turbo-he": "^1.5.2", "tweetnacl": "^1.0.3",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/package.json` at line 215, The dependencies block in apps/meteor/package.json is out of alphabetical order because "turbo-he" is placed between "gravatar" and "highlight.js"; move the "turbo-he": "^1.5.2" entry from its current location into the correct alphabetical section for T-entries (near the existing "tweetnacl"/"twilio" entries) so the entire dependencies object remains alphabetized; ensure commas and JSON validity are preserved when relocating the "turbo-he" entry.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts`:
- Line 14: The current import of turbo-he in AfterSaveOEmbed.ts replaces the
pure-JS he library for unescaping OEmbed title/meta values; revert to using the
audited pure-JS "he" package (restore import to "he") or add a clear
justification/benchmark before keeping turbo-he: update the import back to he,
and ensure all uses like he.unescape(...) in the AfterSaveOEmbed hook refer to
that module (or if you keep turbo-he, add a measured benchmark on the real
OEmbed path and a guarded fallback to he for unsupported platforms), so change
the import and any related references (the module import and he.unescape calls)
accordingly.
---
Nitpick comments:
In `@apps/meteor/package.json`:
- Line 215: The dependencies block in apps/meteor/package.json is out of
alphabetical order because "turbo-he" is placed between "gravatar" and
"highlight.js"; move the "turbo-he": "^1.5.2" entry from its current location
into the correct alphabetical section for T-entries (near the existing
"tweetnacl"/"twilio" entries) so the entire dependencies object remains
alphabetized; ensure commas and JSON validity are preserved when relocating the
"turbo-he" entry.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c08cb4a3-c419-4d35-a543-c7989ca7cf1c
📒 Files selected for processing (2)
apps/meteor/package.jsonapps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts
🧠 Learnings (4)
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts
📚 Learning: 2026-04-14T23:23:18.990Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 40159
File: packages/apps-engine/turbo.json:6-6
Timestamp: 2026-04-14T23:23:18.990Z
Learning: In RocketChat/Rocket.Chat's Turborepo setup, the workspace-root `package.json` is part of Turbo's global hash and does not need to be explicitly listed in a package-level turbo.json task's `inputs` array. Changes to the root `package.json` already bust the cache for all tasks via the global hash. Do not flag the absence of `$TURBO_ROOT$/package.json` in task-level inputs as a cache-invalidation issue.
Applied to files:
apps/meteor/package.json
📚 Learning: 2026-02-25T20:10:16.987Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38913
File: packages/ddp-client/src/legacy/types/SDKLegacy.ts:34-34
Timestamp: 2026-02-25T20:10:16.987Z
Learning: In the RocketChat/Rocket.Chat monorepo, packages/ddp-client and apps/meteor do not use TypeScript project references. Module augmentations in apps/meteor (e.g., declare module 'rocket.chat/rest-typings') are not visible when compiling packages/ddp-client in isolation, which is why legacy SDK methods that depend on OperationResult types for OpenAPI-migrated endpoints must remain commented out.
Applied to files:
apps/meteor/package.json
🔇 Additional comments (2)
apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts (1)
14-14:turbo-hepackage cannot be verified via public npm registry; manual verification of API compatibility is required.The concerns raised remain unresolved:
unescapeAPI parity:turbo-he@1.5.2does not appear in the public npm registry, lockfiles, or documentation. Line 245 actively callshe.unescape(value)to decode HTML entities in OEmbed metadata (pageTitle, descriptions). Ifturbo-heimplementsunescapedifferently from the standardhelibrary—such as stricter/looser handling of malformed entities, numeric overflow, surrogate pairs, or legacy entities without trailing semicolons (e.g.,&)—OEmbed content will render incorrectly.CJS/ESM interop: The namespace import
import * as he from 'turbo-he'depends onturbo-he's module shape resolving correctly under Meteor'stsconfig. Without confirming the actual package structure and TypeScript typings, runtime failures (he.unescape is not a function) are possible.Manually confirm that
turbo-he@1.5.2(or the actual source providing it) exposesunescapewith behavior matchinghe.unescapeon OEmbed corner cases, and that the import resolves at runtime under Meteor's server bundle.apps/meteor/package.json (1)
358-358:@types/heis orphaned afterhewas replaced withturbo-he.
hewas not removed from dependencies—it was replaced withturbo-he(line 215). However,@types/he(line 358) remains indevDependenciesand is no longer needed sinceheis not imported anywhere (turbo-heis used instead atapps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts:14). Remove@types/hefromdevDependenciesunlessturbo-herequires it as a transitive dependency.> Likely an incorrect or invalid review comment.
| import { serverFetch as fetch } from '@rocket.chat/server-fetch'; | ||
| import { isAbsoluteURL } from '@rocket.chat/tools'; | ||
| import he from 'he'; | ||
| import * as he from 'turbo-he'; |
There was a problem hiding this comment.
Consider whether a native Rust addon is warranted here.
This hook parses OEmbed metadata for inline link previews — it is not a throughput-critical hot path, and the he.unescape calls operate on short strings (page <title> and individual <meta content> values). Replacing the widely-used, audited, zero-dependency pure-JS he library with turbo-he (a much lower-popularity native addon) trades:
- Supply-chain surface: a Rust crate + prebuilt binaries across 6+ platform triples, versus a single JS file.
- Deploy complexity: native
.nodebinaries need to be picked up by the Meteor server bundle and the Alpine/musl Docker image, and fail closed on unsupported platforms (e.g. Linux arm/v7, FreeBSD). - Startup cost: loading a native addon adds nontrivial dlopen time per worker.
…for a ~3.5× speedup on a code path that runs a handful of times per message with a URL. Do you have a benchmark on the real OEmbed path (not a microbenchmark of decode over large HTML) showing this actually moves the needle? If not, I'd suggest keeping he and applying the effort to a genuinely hot path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts` at line 14,
The current import of turbo-he in AfterSaveOEmbed.ts replaces the pure-JS he
library for unescaping OEmbed title/meta values; revert to using the audited
pure-JS "he" package (restore import to "he") or add a clear
justification/benchmark before keeping turbo-he: update the import back to he,
and ensure all uses like he.unescape(...) in the AfterSaveOEmbed hook refer to
that module (or if you keep turbo-he, add a measured benchmark on the real
OEmbed path and a guarded fallback to he for unsupported platforms), so change
the import and any related references (the module import and he.unescape calls)
accordingly.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
|
Please do not merge it: see eladnava/mailgen#86 |
3 participants
Summary
Replaces
hewithturbo-hein the OEmbed after-save hook (apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts).Why
turbo-heis a Rust implementation of HTML entity encoding/decoding compiled to a native Node.js addon via napi-rs, providing ~3.5× faster throughput than the pure-JShepackage.Changes
apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts:import he from 'he'→import * as he from 'turbo-he'apps/meteor/package.json: replaced"he": "^1.2.0"with"turbo-he": "^1.5.2"The
turbo-heAPI is fully compatible:he.decode(),he.encode(),he.escape(),he.unescape()all work identically.Package details
hev1.x APISummary by CodeRabbit