Repository navigation
fix(cli): avoid splitting emoji when truncating output - #395
RaphaelFakhri wants to merge 2 commits into
Conversation
dielduarte
left a comment
There was a problem hiding this comment.
Hey @RaphaelFakhri thanks for the contribution, here some feedback before we merge this PR. can you work on them? lmk!!
-
Two truncation sites are missing.
- src/commands/webhooks/utils.ts:53: the Events column in webhooks list still uses eventsStr.slice(0, 57) + '...'. Please switch it to truncate(eventsStr, 60).
- src/commands/careers/apply.ts:348: this file has its own local truncate with the same bug. Please remove it and use the shared helper, keeping the whitespace cleanup at the call site: truncate(value.replace(/\s+/g, ' ').trim(), max).
-
Move the command-level test. The renderReceivingEmailsTable case belongs in tests/commands/emails/receiving/list.test.ts, next to the other tests for that command. tests/lib/truncate.test.ts should only test the helper.
-
Cut on whole visible characters with Intl.Segmenter. The current fix handles simple emoji but still splits emoji built from several characters. At a 50-character cut:
- 👨👩👧 becomes 👨... (a stray joiner character is left)
- 🇺🇸 becomes 🇺... (half a flag)
- 👍🏽 becomes 👍... (the skin tone is silently dropped)
Intl.Segmenter is built in and handles all of these cases. It would also be good to add test cases for a joined-emoji family, a flag and a skin-tone emoji at the cut point.
-
The doc comment says "at most max", which isn't true when max < 3. For example, truncate('abcdef', 2) returns '...'. Either correct the comment, or return value.slice(0, max) when max <= ELLIPSIS.length. No current caller hits this.
Nits:
- The title is fix(emails), but the PR also changes templates and webhooks. Something like fix(cli): avoid splitting emoji when truncating output would fit better.
Use Intl.Segmenter so multi-character emoji are kept or dropped whole, hard-cut when max is not larger than the ellipsis, and use the shared helper in the webhooks Events column and careers apply summary.
ae1ea9d to
eecbeee
Compare
|
Feels good to receive feedback! I'm on it |
dielduarte
left a comment
There was a problem hiding this comment.
Hey @RaphaelFakhri, sorry about this one! I suggested Intl.Segmenter, you implemented it exactly as asked, and it turns out it breaks the standalone binary. That's on me, not you.
What's happening
The resend binary is built with @yao-pkg/pkg (Node 24), which bundles its own Node instead of using the system one. That Node has a cut-down ICU build, and ICU is the library Intl.Segmenter relies on to find character boundaries. new Intl.Segmenter() succeeds, but as soon as it segments a string, the process segfaults.
I reproduced it locally with emails list against a mock API:
| Build | Long subject (truncated) |
|---|---|
main binary |
✅ works |
This PR, run through Node (node dist/cli.cjs) |
✅ works |
| This PR, built binary | ❌ exit 11 (segfault), even with plain ASCII text |
It happens on any truncation, not just emoji. Because it's a native crash, a try/catch or checking whether Intl.Segmenter exists won't help: the constructor works, and the process dies before any fallback could run. CI doesn't catch it because the smoke test only runs --version and --help on the binary.
Let's rethink the approach
The bug this PR set out to fix is the broken � character, from cutting a two-part character (a surrogate pair) in half. Handling multi-part emoji (families, flags, skin tones) was my addition, and it's what pushed us toward ICU. Some options:
for...ofover the string instead ofsegmenter.segment(value). It steps through whole characters, so it can never leave half of one, and it runs in V8 itself, not ICU. I tested it in the binary. It fixes the�and is the fastest and simplest option. The trade-off is that a multi-part emoji at the cut can show as part of itself, like👨instead of the whole family. That's slightly wrong, but it's never a broken character.- A Unicode regex (
\p{RGI_Emoji}with thevflag). It works in the binary and handles official emoji sequences, but it's still an approximation: it splits some scripts, like Devanagari conjuncts, and emoji combinations that aren't official emoji.
I'm leaning toward option 1, since it fixes the real bug with no extra complexity. With that, the family, flag and skin-tone tests would change to check only that there's no unpaired surrogate (not.toMatch(LONE_SURROGATE)) and that the output still ends in .... Open to your thoughts, though!
Sorry again for the detour, and thanks for your patience on this one 🙏
Summary
Several commands shortened long values with
value.slice(0, max - 3) + '...'. When an emoji sat at the cut point,slicecould split it. That left a lone surrogate, which terminals print as a replacement character, or half of a multi-character emoji (a stray joiner, half a flag, or a dropped skin tone).This change adds a shared
truncatehelper insrc/lib/truncate.tsthat cuts on whole visible characters (graphemes) withIntl.Segmenter, so emoji are kept or dropped whole. Whenmaxleaves no room for the ellipsis, the value is hard-cut withslice(0, max).The helper replaces the inline truncation in:
emails listemails receiving get,emails receiving listandemails receiving listentemplates getwebhooks listand webhook event attemptscareers apply(its localtruncateis removed; the whitespace cleanup stays at the call site)Values that fit, and values cut between plain characters, produce the same output as before.
Testing
tests/lib/truncate.test.tscovers values that fit, the length of the cut result, the hard cut whenmaxis 3 or less, and an emoji split at the cut point. It also covers a family emoji, a flag and a skin-tone emoji at every position around the cut.tests/commands/emails/receiving/list.test.tsrenders a receiving-emails table with an emoji subject and checks that the output has no lone surrogate.pnpm test(full suite) andbiome checkpass.