fix: frequently used emoji crash on non-ASCII record ids#7378
fix: frequently used emoji crash on non-ASCII record ids#7378diegolmello wants to merge 3 commits into
Conversation
WalkthroughIncrements DB schema to v29 and adds a migration that deletes legacy non-ASCII-ID emoji rows; refactors frequently-used emoji storage to use (content, is_custom) with exported getFrequentlyUsedEmojis; updates useFrequentlyUsedEmoji to call the helper; adds Jest tests for methods, hook, and migration. ChangesEmoji ID Corruption Fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labelstype: bug Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/lib/hooks/useFrequentlyUsedEmoji.ts (1)
14-21: ⚡ Quick winHandle the effect's async work explicitly.
Line 20 starts an async function and drops the returned promise, and this effect has no cleanup guard if
withDefaultEmojischanges or the component unmounts mid-fetch. Wrap the await intry/catch, handle the promise explicitly from the effect entrypoint, and skipsetFrequentlyUsed/setLoadedonce the effect is stale.Suggested change
useEffect(() => { + let cancelled = false; const fetchFrequentlyUsedEmojis = async () => { - const emojis = await getFrequentlyUsedEmojis(withDefaultEmojis); - setFrequentlyUsed(emojis); - setLoaded(true); + try { + const emojis = await getFrequentlyUsedEmojis(withDefaultEmojis); + if (cancelled) { + return; + } + setFrequentlyUsed(emojis); + setLoaded(true); + } catch { + if (!cancelled) { + setLoaded(true); + } + } }; - fetchFrequentlyUsedEmojis(); + fetchFrequentlyUsedEmojis().catch(() => undefined); + return () => { + cancelled = true; + }; }, [withDefaultEmojis]);Based on learnings, the repo enforces
no-void: error, and as per coding guidelines async operations should use explicit error handling with try/catch blocks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/lib/hooks/useFrequentlyUsedEmoji.ts` around lines 14 - 21, The effect in useFrequentlyUsedEmoji (useEffect -> fetchFrequentlyUsedEmojis) starts an async task and drops the promise without error handling or a stale-check; change the effect so it calls the async work explicitly (do not "void" the promise), wrap the await getFrequentlyUsedEmojis(withDefaultEmojis) in try/catch, and add a cancellation guard (e.g., a mounted/aborted flag or AbortController) in the effect cleanup so that setFrequentlyUsed and setLoaded are skipped if the effect became stale or the component unmounted; keep references to the existing symbols fetchFrequentlyUsedEmojis, useEffect, getFrequentlyUsedEmojis, setFrequentlyUsed, setLoaded, and withDefaultEmojis when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/lib/methods/emojis.ts`:
- Around line 27-49: The lookup for an existing frequently-used emoji is done
before entering the serialized db.write(), allowing a race that creates
duplicate rows; move the query into the db.write() closure so the
check-and-create/update happens atomically under the same serialized transaction
(i.e., inside db.write() perform freqEmojiCollection.query(...).fetch(), inspect
the resulting existing record, and then either update its count or create a new
record), referencing the existing symbols freqEmojiCollection, addFrequentlyUsed
(the caller), db.write, and the existing variable; also add a regression test
that calls addFrequentlyUsed twice in parallel and asserts a single row exists
with count === 2.
---
Nitpick comments:
In `@app/lib/hooks/useFrequentlyUsedEmoji.ts`:
- Around line 14-21: The effect in useFrequentlyUsedEmoji (useEffect ->
fetchFrequentlyUsedEmojis) starts an async task and drops the promise without
error handling or a stale-check; change the effect so it calls the async work
explicitly (do not "void" the promise), wrap the await
getFrequentlyUsedEmojis(withDefaultEmojis) in try/catch, and add a cancellation
guard (e.g., a mounted/aborted flag or AbortController) in the effect cleanup so
that setFrequentlyUsed and setLoaded are skipped if the effect became stale or
the component unmounted; keep references to the existing symbols
fetchFrequentlyUsedEmojis, useEffect, getFrequentlyUsedEmojis,
setFrequentlyUsed, setLoaded, and withDefaultEmojis when making the change.
🪄 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: eb156926-4217-4b71-9b78-ca5e5dc02a72
📒 Files selected for processing (5)
app/lib/database/model/migrations.jsapp/lib/database/schema/app.jsapp/lib/hooks/useFrequentlyUsedEmoji.tsapp/lib/methods/emojis.test.tsapp/lib/methods/emojis.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use descriptive names for functions, variables, and classes that clearly convey their purpose
Write comments that explain the 'why' behind code decisions, not the 'what'
Keep functions small and focused on a single responsibility
Use const by default, let when reassignment is needed, and avoid var
Prefer async/await over .then() chains for handling asynchronous operations
Use explicit error handling with try/catch blocks for async operations
Avoid deeply nested code; refactor complex logic into helper functions
Files:
app/lib/database/schema/app.jsapp/lib/methods/emojis.test.tsapp/lib/methods/emojis.tsapp/lib/hooks/useFrequentlyUsedEmoji.tsapp/lib/database/model/migrations.js
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Use Prettier with tabs, single quotes, 130 char width, no trailing commas, arrow parens avoid, bracket same line
Use@rocket.chat/eslint-configbase with React, React Native, TypeScript, Jest plugins
Files:
app/lib/database/schema/app.jsapp/lib/methods/emojis.test.tsapp/lib/methods/emojis.tsapp/lib/hooks/useFrequentlyUsedEmoji.tsapp/lib/database/model/migrations.js
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbersUse TypeScript with strict mode and baseUrl set to app/ for import resolution
Files:
app/lib/methods/emojis.test.tsapp/lib/methods/emojis.tsapp/lib/hooks/useFrequentlyUsedEmoji.ts
🧠 Learnings (1)
📚 Learning: 2026-04-30T17:07:51.020Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7274
File: app/lib/services/voip/MediaCallEvents.ts:0-0
Timestamp: 2026-04-30T17:07:51.020Z
Learning: In this Rocket.Chat React Native codebase, the ESLint rule `no-void: error` is enforced. When you see a promise returned from an async call that is not awaited (a “floating promise”), do not silence it with the `void somePromise()` pattern. Instead, handle the promise explicitly by attaching `.catch(...)` (or otherwise awaiting/handling the error) so unhandled-rejection risks are addressed in a way that satisfies the existing ESLint configuration.
Applied to files:
app/lib/methods/emojis.test.tsapp/lib/methods/emojis.tsapp/lib/hooks/useFrequentlyUsedEmoji.ts
🔇 Additional comments (4)
app/lib/database/schema/app.js (1)
4-4: LGTM!app/lib/database/model/migrations.js (1)
1-1: LGTM!Also applies to: 349-358
app/lib/methods/emojis.ts (1)
55-80: LGTM!app/lib/methods/emojis.test.ts (1)
1-129: LGTM!
addFrequentlyUsed used the emoji content / custom-emoji name as the WatermelonDB record id. Custom emoji names are server-controlled and can be non-ASCII (CJK, ZWJ sequences). Non-ASCII ids do not round-trip across the native SQLite/JSI bridge: the adapter's cached-id set desyncs from the JS RecordCache, so the entire frequently_used_emojis .fetch() throws "Record ID ... was sent over the bridge, but it's not cached" and crashes every emoji surface (picker, composer search, reaction bar). - Let WatermelonDB own a safe ASCII record id; dedup by the content + is_custom fields instead of find-by-id. - Centralize the read in a resilient getFrequentlyUsedEmojis that logs and degrades to defaults on a bridge error rather than crashing the UI. - Add a v29 migration purging legacy rows whose id contains a non-printable-ASCII character (keeps ASCII shortname ids like heart_eyes). - Cover with mocked-database.active unit tests.
8758752 to
d4f6fa5
Compare
Proposed changes
addFrequentlyUsedused the emoji content / custom-emoji name as the WatermelonDB record id. Custom emoji names are server-controlled and can be non-ASCII (CJK, ZWJ sequences). Non-ASCII ids do not round-trip across the native SQLite/JSI bridge: the adapter's cached-id set desyncs from the JSRecordCache, so the entirefrequently_used_emojis.fetch()rejects withRecord ID ... was sent over the bridge, but it's not cachedand crashes every emoji surface (picker, composer search, reaction bar). Because results map synchronously, a single bad row takes down the whole fetch.This PR:
content+is_customfields instead offind-by-id (content stays a plain column value, which round-trips fine).db.write()so the find-or-create is serialized — two concurrentaddFrequentlyUsedcalls can no longer both miss the row and insert a duplicate.getFrequentlyUsedEmojisthat logs and degrades to defaults on a bridge error instead of crashing the UI.id GLOB '*[^ -~]*'), so ASCII shortname ids such asheart_eyesare preserved.database.active, the repo's existing DB-test pattern) for the method and theuseFrequentlyUsedEmojihook.For contrast,
getCustomEmojiswas never affected — it correctly keyscustom_emojisby the server_id(ASCII-safe).Issue(s)
https://rocketchat.atlassian.net/browse/NATIVE-1192
How to test or reproduce
This crash only manifests with the native SQLite/JSI adapter — it cannot be reproduced in pure JS (non-ASCII ids round-trip fine there), so it needs a device/emulator build.
About the trigger: the crash requires a custom emoji whose name is non-ASCII (CJK, Cyrillic, emoji glyphs, etc.). Modern Rocket.Chat servers slugify custom emoji names to ASCII on create (
大丈夫→da4_zhang4_fu1), so you cannot create a triggering emoji through Admin → Emoji there. To reproduce, use a server that stores non-ASCII names verbatim — an older Rocket.Chat, or any workspace that already carries such an emoji as legacy data.Reproduce the crash (pre-fix build, non-slugifying server)
大丈夫), open any channel.frequently_used_emojis.Record ID frequently_used_emojis#大丈夫 was sent over the bridge, but it's not cachedand every emoji surface fails to load.Verify the fix (this branch, same server)
:name:), the reaction badge, the picker grid, and the frequently-used row.Verify the upgrade heals existing installs
heart_eyesare kept).Regression check (any server)
Automated coverage
TZ=UTC pnpm test --testPathPattern='app/lib/(methods/emojis|hooks/useFrequentlyUsedEmoji).test'addFrequentlyUsedcreates via a content + is_custom query, never sets the emoji as the id, increments count on repeat, and runs its lookup inside the serialized write (no read-before-write race).getFrequentlyUsedEmojismaps row shapes and returns[]/ defaults instead of throwing when a bridge desync is simulated.useFrequentlyUsedEmojiexposes the fetched emojis and flipsloaded, and refetches whenwithDefaultEmojischanges.Screenshots
N/A — crash fix, no visual change.
Types of changes
Checklist
Further comments
The migration heals existing broken installs once at upgrade; the safe-id change prevents new bad rows; the resilient read is belt-and-suspenders so any unforeseen desync can never crash the emoji UI again.
Out of scope (flagged for follow-up): the same content-as-id anti-pattern exists, lower-risk, in
getSlashCommands(command.command) andsendFileMessage(uploadPath, can contain non-ASCII filename chars). Every othersanitizedRaw({ id })site uses a server_id/rid, which is ASCII-safe.Summary by CodeRabbit
New Features
Bug Fixes
Tests