Skip to content

Conversation

@Rohit3523
Copy link
Collaborator

@Rohit3523 Rohit3523 commented Sep 3, 2025

Proposed changes

Issue(s)

https://rocketchat.atlassian.net/browse/COMM-65

How to test or reproduce

Screenshots

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • Improvement (non-breaking change which improves a current function)
  • New feature (non-breaking change which adds functionality)
  • Documentation update (if none of the other choices apply)

Checklist

  • I have read the CONTRIBUTING doc
  • I have signed the CLA
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works (if applicable)
  • I have added necessary documentation (if applicable)
  • Any dependent changes have been merged and published in downstream modules

Further comments

Summary by CodeRabbit

  • Refactor
    • Migrated the app’s icon system to a new vector-icon provider; no visual changes expected.
  • Bug Fixes
    • Improved icon presence checks to prevent missing icons; existing fallbacks preserved.
  • Documentation
    • Updated icon usage guide to reflect the new icon setup and provider.
  • Chores
    • Removed unused icon font resources from the iOS build and updated project dependencies.

✏️ Tip: You can customize this high-level summary in your review settings.

@Rohit3523 Rohit3523 changed the title Warning fix chore: replace react-native-vector-icon with @expo-icon Sep 3, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 10, 2025

Caution

Review failed

The pull request is closed.

Walkthrough

Replaces react-native-vector-icons with @expo/vector-icons; CustomIcon now builds a glyphMap and exports hasIcon; Status calls hasIcon with a fallback to status-offline; iOS project removed vector icon TTFs; package.json and docs updated.

Changes

Cohort / File(s) Summary of Changes
Icon component and API
app/containers/CustomIcon/index.tsx
Replaced react-native-vector-icons usage with @expo/vector-icons; switched selection import to ES default; compute glyphMap (use IconSet.getRawGlyphMap() when available, fallback to selection JSON); export hasIcon(name) helper; update ICustomIcon to extend React.ComponentProps<typeof IconSet> and add optional style; continue exporting IconSet and CustomIcon.
Consumer update
app/containers/Status/Status.tsx
Use imported hasIcon(name) instead of IconSet.hasIcon(name) and preserve fallback to 'status-offline'.
Dependencies
package.json
Added @expo/vector-icons; removed react-native-vector-icons and @types/react-native-vector-icons.
iOS resources cleanup
ios/.../project.pbxproj
Removed multiple vector-icon *.ttf entries from Copy Pods Resources across targets.
Documentation
docs/icons.md
Updated docs to reference @expo/vector-icons and adjusted IcoMoon wording/link.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor U as User
  participant S as Status component
  participant CI as CustomIcon module
  participant GM as glyphMap

  U->>S: render Status(iconName)
  S->>CI: call hasIcon(iconName)
  CI->>GM: lookup iconName
  GM-->>CI: found? (true/false)
  CI-->>S: return boolean
  alt icon exists
    S->>S: render provided iconName
  else icon missing
    S->>S: render "status-offline"
  end
  S-->>U: UI rendered
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Review focus:
    • app/containers/CustomIcon/index.tsx — glyphMap construction, IconSet.getRawGlyphMap() usage, hasIcon correctness, and typing change to React.ComponentProps<typeof IconSet>.
    • ios/.../project.pbxproj — ensure font removals don't break other targets or resource expectations.
    • package.json — dependency swap and CI implications.

Poem

I hop through glyphs with tidy cheer,
Old fonts burrowed, new sprites near.
I peek each name with hasIcon's eye,
If missing, "offline" I softly try.
A rabbit's nod — vector hops high. 🥕

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'chore: migrate from react-native-vector-icons to @expo/vector-icons' clearly and concisely describes the main change—migration from one icon library to another.
Linked Issues check ✅ Passed The PR successfully implements the primary objective from COMM-65 by replacing react-native-vector-icons with @expo/vector-icons across all affected files and dependencies.
Out of Scope Changes check ✅ Passed All changes are directly related to the icon library migration objective. Updates to CustomIcon, Status, documentation, iOS project file, and package.json are all necessary and scoped.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between b430837 and 2718395.

⛔ Files ignored due to path filters (2)
  • ios/Podfile.lock is excluded by !**/*.lock
  • yarn.lock is excluded by !**/yarn.lock, !**/*.lock
📒 Files selected for processing (2)
  • ios/RocketChatRN.xcodeproj/project.pbxproj (0 hunks)
  • package.json (1 hunks)

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@Rohit3523 Rohit3523 temporarily deployed to approve_e2e_testing October 10, 2025 19:12 — with GitHub Actions Inactive
@Rohit3523 Rohit3523 had a problem deploying to experimental_android_build October 10, 2025 19:16 — with GitHub Actions Error
@Rohit3523 Rohit3523 had a problem deploying to experimental_ios_build October 10, 2025 19:16 — with GitHub Actions Error
@Rohit3523 Rohit3523 had a problem deploying to official_android_build October 10, 2025 19:16 — with GitHub Actions Error
@Rohit3523 Rohit3523 marked this pull request as ready for review October 10, 2025 20:15
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 53e26da and 28b75f0.

⛔ Files ignored due to path filters (2)
  • ios/Podfile.lock is excluded by !**/*.lock
  • yarn.lock is excluded by !**/yarn.lock, !**/*.lock
📒 Files selected for processing (5)
  • app/containers/CustomIcon/index.tsx (1 hunks)
  • app/containers/Status/Status.tsx (2 hunks)
  • docs/icons.md (1 hunks)
  • ios/RocketChatRN.xcodeproj/project.pbxproj (0 hunks)
  • package.json (1 hunks)
💤 Files with no reviewable changes (1)
  • ios/RocketChatRN.xcodeproj/project.pbxproj
🧰 Additional context used
🧬 Code graph analysis (2)
app/containers/Status/Status.tsx (1)
app/containers/CustomIcon/index.tsx (1)
  • hasIcon (19-19)
app/containers/CustomIcon/index.tsx (1)
app/containers/CustomIcon/mappedIcons.js (2)
  • mappedIcons (1-229)
  • mappedIcons (1-229)
🪛 markdownlint-cli2 (0.18.1)
docs/icons.md

3-3: Bare URL used

(MD034, no-bare-urls)

⏰ 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). (2)
  • GitHub Check: ESLint and Test / run-eslint-and-test
  • GitHub Check: format

Comment on lines +12 to +17
const glyphMap = IconSet.getRawGlyphMap
? IconSet.getRawGlyphMap()
: icoMoonConfig.icons?.reduce((map: Record<string, string | number>, glyph: any) => {
map[glyph.icon.name] = glyph.icon.code;
return map;
}, {}) || {};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Fix fallback glyph map extraction.

icoMoonConfig.icons stores metadata under properties, not icon, so the fallback map is always empty. If IconSet.getRawGlyphMap is unavailable, hasIcon will return false for every name and regress status icons. Please pull the name/code from glyph.properties.

Apply this diff:

-const glyphMap = IconSet.getRawGlyphMap
-	? IconSet.getRawGlyphMap()
-	: icoMoonConfig.icons?.reduce((map: Record<string, string | number>, glyph: any) => {
-			map[glyph.icon.name] = glyph.icon.code;
-			return map;
-	  }, {}) || {};
+const glyphMap =
+	IconSet.getRawGlyphMap?.() ??
+	(icoMoonConfig.icons ?? []).reduce((map: Record<string, string | number>, glyph: any) => {
+		const name = glyph?.properties?.name;
+		if (name) {
+			map[name] = glyph?.properties?.code ?? map[name];
+		}
+		return map;
+	}, {});
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const glyphMap = IconSet.getRawGlyphMap
? IconSet.getRawGlyphMap()
: icoMoonConfig.icons?.reduce((map: Record<string, string | number>, glyph: any) => {
map[glyph.icon.name] = glyph.icon.code;
return map;
}, {}) || {};
const glyphMap =
IconSet.getRawGlyphMap?.() ??
(icoMoonConfig.icons ?? []).reduce((map: Record<string, string | number>, glyph: any) => {
const name = glyph?.properties?.name;
if (name) {
map[name] = glyph?.properties?.code ?? map[name];
}
return map;
}, {});
🤖 Prompt for AI Agents
In app/containers/CustomIcon/index.tsx around lines 12 to 17, the fallback glyph
map reads name/code from glyph.icon but icoMoonConfig.icons stores metadata
under glyph.properties, so the fallback map is always empty; update the reduce
to pull name and code from glyph.properties (e.g., glyph.properties.name and
glyph.properties.code), keeping the same map type and defaulting to {} so
hasIcon works when IconSet.getRawGlyphMap is unavailable.

# Icons

Icons are generated using IcoMoon and react-native-vector-icons https://github.com/oblador/react-native-vector-icons#createiconsetfromicomoonconfig-fontfamily-fontfile
Icons are generated using IcoMoon and `@expo/vector-icons` https://docs.expo.dev/guides/icons/#createiconsetfromicomoon
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Wrap bare URL for markdownlint compliance.

Markdown lint (MD034) flags this bare URL; wrap it like <https://docs.expo.dev/guides/icons/#createiconsetfromicomoon> to satisfy the check.

🧰 Tools
🪛 markdownlint-cli2 (0.18.1)

3-3: Bare URL used

(MD034, no-bare-urls)

🤖 Prompt for AI Agents
In docs/icons.md around line 3, the markdown contains a bare URL which triggers
MD034; wrap the URL in angle brackets to make it inline code-compliant by
replacing the raw link with the bracketed form
<https://docs.expo.dev/guides/icons/#createiconsetfromicomoon> so markdownlint
stops flagging it.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 53e26da and 28b75f0.

⛔ Files ignored due to path filters (2)
  • ios/Podfile.lock is excluded by !**/*.lock
  • yarn.lock is excluded by !**/yarn.lock, !**/*.lock
📒 Files selected for processing (5)
  • app/containers/CustomIcon/index.tsx (1 hunks)
  • app/containers/Status/Status.tsx (2 hunks)
  • docs/icons.md (1 hunks)
  • ios/RocketChatRN.xcodeproj/project.pbxproj (0 hunks)
  • package.json (1 hunks)
💤 Files with no reviewable changes (1)
  • ios/RocketChatRN.xcodeproj/project.pbxproj
🧰 Additional context used
🧬 Code graph analysis (2)
app/containers/CustomIcon/index.tsx (2)
app/containers/CustomIcon/mappedIcons.js (2)
  • mappedIcons (1-229)
  • mappedIcons (1-229)
scripts/build-icon-set.js (1)
  • mappedIcons (19-19)
app/containers/Status/Status.tsx (1)
app/containers/CustomIcon/index.tsx (1)
  • hasIcon (19-19)
🪛 markdownlint-cli2 (0.18.1)
docs/icons.md

3-3: Bare URL used

(MD034, no-bare-urls)

⏰ 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). (2)
  • GitHub Check: ESLint and Test / run-eslint-and-test
  • GitHub Check: format

Comment on lines +14 to +17
: icoMoonConfig.icons?.reduce((map: Record<string, string | number>, glyph: any) => {
map[glyph.icon.name] = glyph.icon.code;
return map;
}, {}) || {};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Fix glyphMap fallback to read metadata from properties.

selection.json stores the icon name/code under glyph.properties, not glyph.icon. This fallback currently builds a map with undefined keys/values, so hasIcon() always returns false whenever IconSet.getRawGlyphMap isn’t available (which happens with @expo/vector-icons). That breaks every icon guard, e.g. statuses always falling back to status-offline. Please read the metadata from properties (and guard for missing codes).

-	: icoMoonConfig.icons?.reduce((map: Record<string, string | number>, glyph: any) => {
-			map[glyph.icon.name] = glyph.icon.code;
-			return map;
-	  }, {}) || {};
+	: icoMoonConfig.icons?.reduce((map: Record<string, string | number>, glyph: any) => {
+			const name = glyph?.properties?.name;
+			const code = glyph?.properties?.code;
+			if (typeof name === 'string') {
+				map[name] = code ?? mappedIcons[name];
+			}
+			return map;
+	  }, {}) || {};
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
: icoMoonConfig.icons?.reduce((map: Record<string, string | number>, glyph: any) => {
map[glyph.icon.name] = glyph.icon.code;
return map;
}, {}) || {};
: icoMoonConfig.icons?.reduce((map: Record<string, string | number>, glyph: any) => {
const name = glyph?.properties?.name;
const code = glyph?.properties?.code;
if (typeof name === 'string') {
map[name] = code ?? mappedIcons[name];
}
return map;
}, {}) || {};
🤖 Prompt for AI Agents
In app/containers/CustomIcon/index.tsx around lines 14–17, the fallback glyphMap
reads from glyph.icon but selection.json stores metadata under glyph.properties;
update the reducer to read the icon name and code from glyph.properties (e.g.
properties.name and properties.code or properties.codepoint/unicode as
available), only add entries when a valid name and code exist, and coerce the
code to the same type expected by other callers (string or number) so missing
codes are skipped instead of inserting undefined keys/values.

# Icons

Icons are generated using IcoMoon and react-native-vector-icons https://github.com/oblador/react-native-vector-icons#createiconsetfromicomoonconfig-fontfamily-fontfile
Icons are generated using IcoMoon and `@expo/vector-icons` https://docs.expo.dev/guides/icons/#createiconsetfromicomoon
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Wrap the link to satisfy markdownlint.

markdownlint (MD034) flags the bare URL. Please wrap it in angle brackets or convert it into markdown link syntax to keep docs lint passing.

🧰 Tools
🪛 markdownlint-cli2 (0.18.1)

3-3: Bare URL used

(MD034, no-bare-urls)

🤖 Prompt for AI Agents
docs/icons.md around line 3: the bare URL triggers markdownlint MD034; update
the text to wrap the URL in angle brackets or convert it into markdown link
syntax (e.g., [Expo icons
guide](https://docs.expo.dev/guides/icons/#createiconsetfromicomoon)) so the
link is no longer a bare URL and the markdown linter passes.

@diegolmello diegolmello changed the title chore: replace react-native-vector-icon with @expo-icon chore: migrate from react-native-vector-icons to @expo/vector-icons Nov 5, 2025
@Rohit3523 Rohit3523 temporarily deployed to approve_e2e_testing November 5, 2025 14:18 — with GitHub Actions Inactive
@Rohit3523 Rohit3523 had a problem deploying to experimental_ios_build November 5, 2025 14:23 — with GitHub Actions Error
@Rohit3523 Rohit3523 had a problem deploying to experimental_android_build November 5, 2025 14:23 — with GitHub Actions Error
@Rohit3523 Rohit3523 had a problem deploying to official_android_build November 5, 2025 14:23 — with GitHub Actions Error
@Rohit3523
Copy link
Collaborator Author

Conflict Resolved

@diegolmello diegolmello merged commit 91967fc into develop Dec 3, 2025
4 of 6 checks passed
@diegolmello diegolmello deleted the warning-fix branch December 3, 2025 20:05
@diegolmello diegolmello requested a deployment to experimental_ios_build December 3, 2025 20:08 — with GitHub Actions Waiting
@diegolmello diegolmello requested a deployment to experimental_android_build December 3, 2025 20:08 — with GitHub Actions Waiting
@diegolmello diegolmello requested a deployment to official_android_build December 3, 2025 20:08 — with GitHub Actions Waiting
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants