fix(desktop): render text reaction fallbacks safely - #4996
fix(desktop): render text reaction fallbacks safely#4996AaronGoldsmith wants to merge 4 commits into
Conversation
Co-authored-by: Bozo <c52c3b2f8ad8558a64330f4f9929518e12d500f76e2d7d3aca75ebe7f7a79ea7@buzz.block.builderlab.xyz> Signed-off-by: Bozo <c52c3b2f8ad8558a64330f4f9929518e12d500f76e2d7d3aca75ebe7f7a79ea7@buzz.block.builderlab.xyz>
Co-authored-by: Bozo <c52c3b2f8ad8558a64330f4f9929518e12d500f76e2d7d3aca75ebe7f7a79ea7@buzz.block.builderlab.xyz> Signed-off-by: Bozo <c52c3b2f8ad8558a64330f4f9929518e12d500f76e2d7d3aca75ebe7f7a79ea7@buzz.block.builderlab.xyz>
Co-authored-by: Bozo <c52c3b2f8ad8558a64330f4f9929518e12d500f76e2d7d3aca75ebe7f7a79ea7@buzz.block.builderlab.xyz> Signed-off-by: Bozo <c52c3b2f8ad8558a64330f4f9929518e12d500f76e2d7d3aca75ebe7f7a79ea7@buzz.block.builderlab.xyz>
Co-authored-by: SmartyP <bc4b2dcdb5359ccde0d49f65d1dda695890634a6a3bcc82346e38232934a75d9@buzz.block.builderlab.xyz> Co-authored-by: Smarty <bc4b2dcdb5359ccde0d49f65d1dda695890634a6a3bcc82346e38232934a75d9@buzz.block.builderlab.xyz> Signed-off-by: SmartyP <bc4b2dcdb5359ccde0d49f65d1dda695890634a6a3bcc82346e38232934a75d9@buzz.block.builderlab.xyz>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e051586218
ℹ️ 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".
| } | ||
|
|
||
| function isNativeEmojiCluster(cluster: string): boolean { | ||
| export function isNativeEmojiCluster(cluster: string): boolean { |
There was a problem hiding this comment.
Document or stop exporting the emoji helpers
This commit turns both isNativeEmojiCluster and readGrapheme into exported APIs without adding doc comments, even though both remain implementation details used only within emojiOnly.ts. Keep them private or document the contract and edge-case behavior of each exported helper as required for new public APIs.
AGENTS.md reference: AGENTS.md:L116-L116
Useful? React with 👍 / 👎.
|
🤖 Reviewed on behalf of @ravarora2. Overall this looks solid — desktop-only, honestly scoped, fails safe (a misclassification only nudges a glyph between the tight box and the roomy text box; image emoji, toggle identity, titles, and a11y all use the raw value), and the unit + e2e coverage locks the important cases. Two notes, both non-blocking. 1. Agree with the Codex P1 on the new exports
2. Consider the platform primitives for the emoji test (follow-up)The detection works and fails safe, so this isn't a merge blocker — but the two brittle pieces are (a) the hand-rolled grapheme reader
Roughly: const GRAPHEME_SEGMENTER = new Intl.Segmenter(undefined, { granularity: "grapheme" });
export function isSingleNativeEmoji(value: string): boolean {
if (!value) return false;
if ([...GRAPHEME_SEGMENTER.segment(value)].length !== 1) return false;
return /^\p{RGI_Emoji}$/v.test(value);
}Caveat / why it's a follow-up not a blocker: Tauri uses the OS webview, and Nice edge-case handling on the malformed ZWJ, by the way. |
Summary
:missing_reaction:displays asmissing_reaction), while preserving raw reaction content for titles, accessibility, and toggle identity.emojiUrlbranch.Kind-7 reaction content is literal under NIP-25, so existing or legacy arbitrary-text events retain their meaning and are rendered safely rather than rejected. NIP-30's matching
emojitag / resolvedemojiUrlremains the only custom-image signal.This is intentionally different from:
This PR is Desktop-only: it changes no relay ingest, persistence, protocol, or channel derivation. A possible CLI warning for an unknown
:shortcode:sent without--emoji-urlis intentionally deferred and not coupled to rendering compatibility.Reproduction
The resulting kind-7 event has no matching NIP-30
emojitag, so Desktop must treat the content as a text fallback rather than forcing it into a native-emoji box.Testing
pnpm biome check src/features/messages/ui/MessageReactions.tsx src/features/messages/lib/reactionGlyphPresentation.ts src/features/messages/lib/reactionGlyphPresentation.test.mjs src/shared/lib/emojiOnly.ts tests/e2e/reaction-names.spec.tspnpm test(4,373 passed)pnpm build:e2epnpm exec playwright test tests/e2e/reaction-names.spec.ts(3 passed), covering:ship itpill + hover popoversq agents review main...HEAD --local ...after addressing its malformed-ZWJ finding (final local review: no findings)Screenshots from Testing
Pill row: unresolved shortcode, arbitrary text, and long literal text — each bounded, left-aligned, and truncated without overlapping its count
Original UI rendering bug example