Fix emoji skin tone type mismatch when stored as string#82021
Conversation
When the server returns preferredEmojiSkinTone as a string (e.g., "2") instead of a number (2), two bugs occur: 1. getSkinToneEmojiFromIndex uses strict === comparison, so "2" !== 2 fails to match, falling back to the default skin tone display. 2. usePreferredEmojiSkinTone compares Number(stored) === Number(new), which returns true (both are 2), silently blocking any update attempt. Fix: Cast the stored value to Number early in both locations so the comparison and display work correctly regardless of the stored type. Fixes Expensify#81909 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
melvin-bot
Bot
requested review from
cead22 and
trjExpensify
and removed request for
a team
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
![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: deda637db3
ℹ️ 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".
| export default function usePreferredEmojiSkinTone() { | ||
| const [preferredSkinTone = CONST.EMOJI_DEFAULT_SKIN_TONE] = useOnyx(ONYXKEYS.PREFERRED_EMOJI_SKIN_TONE, {canBeMissing: true}); | ||
|
|
||
| const numericPreferredSkinTone = Number(preferredSkinTone); |
There was a problem hiding this comment.
Coercing preferredSkinTone with Number(...) unconditionally turns malformed cached values into seemingly valid numbers (for example '' or null become 0, and non-numeric strings become NaN). Downstream emoji picker code (e.g., EmojiPickerMenu/index.tsx and index.native.tsx) gates on typeof preferredSkinTone === 'number', so this change can now route invalid data through the skin-tone path and render the wrong variant instead of falling back to the default tone; previously those malformed values stayed non-numeric and safely fell back. Please guard the conversion (Number.isFinite/range check) and fall back to CONST.EMOJI_DEFAULT_SKIN_TONE when invalid.
Useful? React with 👍 / 👎.
|
@trjExpensify @cead22 please ignore this PR. The contributor didn't follow process. Not assigned in the linked issue. |
|
Apologies — I submitted this PR prematurely before being assigned to the issue. Closing per the contributing guidelines. The proposal is still on the issue and the fix is ready to go once assigned. |
2 participants
Summary
$ #81909
When the server returns
preferredEmojiSkinToneas a string (e.g.,"2") instead of a number (2), two bugs occur:getSkinToneEmojiFromIndex.ts— Uses strict===comparison, so"2" !== 2fails to match any skin tone, falling back to the default yellow hand display.usePreferredEmojiSkinTone.ts— ComparesNumber(stored) === Number(new), which returnstrue(both are2), silently blocking any update attempt. The user sees the wrong skin tone and cannot change it.Fix
Numberearly in both locationsgetSkinToneEmojiFromIndex:emoji.skinTone === Number(skinToneIndex)ensures the lookup works regardless of stored typeusePreferredEmojiSkinTone: Normalize tonumericPreferredSkinToneonce, use it for comparison and return value — ensures downstream consumers always get a numberFixed Issues
$ #81909
Tests
Onyx.merge('preferredEmojiSkinTone', "2")preferredEmojiSkinToneto number3viaOnyx.merge('preferredEmojiSkinTone', 3)→ verify it works as before (no regression)Offline tests
QA Steps
Onyx.merge('preferredEmojiSkinTone', "4")in DevTools consolePR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Web
Before fix (string
"2"stored): Default yellow hand shown, cannot change skin toneAfter fix (string
"2"stored): Correct cream/light hand shown, skin tone change worksThis is a type-coercion fix with no visual UI changes — the emoji picker displays the correct skin tone that was already stored.