Repository navigation
Remove unsafe assertions from badge and theme style defaults - #100026
Conversation
Seatbelt-Operation: main-update-100026-20261016-sign-5b2aba3a
|
@ikevin127 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const bgColor = (backgroundColor ?? theme.transparent) as string; | ||
| const txtColor = (textColor ?? theme.text) as string; | ||
| const textColorStyle: TextStyle = {color: textColor ?? theme.text}; | ||
|
|
There was a problem hiding this comment.
❌ UI-5 (docs)
This hand-builds {color: textColor ?? theme.text} as a TextStyle, but an existing StyleUtils helper already produces exactly this shape: getColorStyle(color: string) returns {color}. Duplicating the helper's logic inline bypasses the shared style system and means future changes to how a color style is produced won't reach this call site.
Reuse the existing helper, keeping the previous usage:
<Badge
text={text}
isCondensed
badgeStyles={[styles.ml0, styles.borderNone, StyleUtils.getBackgroundColorStyle(backgroundColor ?? theme.transparent), badgeStyles]}
textStyles={StyleUtils.getColorStyle(textColor ?? theme.text)}
/>If the goal was to drop the as string cast, prefer widening getColorStyle's parameter to ColorValue over rebuilding the object literal here, so the helper stays the single source of truth.
Reviewed at: fdf5b40 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Reused StyleUtils.getColorStyle in StatusBadge. Updated the shared helper and TextColorStyle to accept React Native ColorValue, so no string cast is needed.
Reviewer Checklist
Screenshots/VideosScreen.Recording.2026-10-16.at.19.30.36.mov |
|
|
||
| const bgColor = (backgroundColor ?? theme.transparent) as string; | ||
| const txtColor = (textColor ?? theme.text) as string; | ||
| const textColorStyle: TextStyle = {color: textColor ?? theme.text}; |
There was a problem hiding this comment.
src/components/StatusBadge.tsx L28
🟡 Echoing the UI-5 bot comment here, but going back to getColorStyle(txtColor) would need the as string again, because getColorStyle only accepts string while textColor is a ColorValue.
The real fix is to widen the helper so it matches getBackgroundColorStyle(backgroundColor: ColorValue):
// src/styles/utils/types.ts
type TextColorStyle = {color: ColorValue};
// src/styles/utils/index.ts
function getColorStyle(color: ColorValue): TextColorStyle {
return {color};
}textStyles={StyleUtils.getColorStyle(textColor ?? theme.text)}This works for every current caller (they all pass the result into a style array), and it means the next ColorValue caller won't need another cast or another inline object.
There was a problem hiding this comment.
Updated getColorStyle and TextColorStyle to use React Native ColorValue, then reused the helper in StatusBadge. Kept getTextColorStyle's inferred string-only result for its web CSS consumers. The focused type check includes the actual MapView consumer and passes.
| get styles() { | ||
| if (!cachedState) { | ||
| cachedState = {styles: styles(defaultTheme)}; | ||
| } | ||
| return cachedState[prop]; | ||
| return cachedState.styles; | ||
| }, | ||
| }); | ||
| }; | ||
|
|
||
| const defaultThemeStylesActionsContextValue = new Proxy({} as ThemeStylesActionsContextType, { | ||
| get(_, prop: keyof ThemeStylesActionsContextType) { | ||
| const defaultThemeStylesActionsContextValue: ThemeStylesActionsContextType = { | ||
| get StyleUtils() { | ||
| if (!cachedActions) { | ||
| if (!cachedState) { | ||
| cachedState = {styles: styles(defaultTheme)}; | ||
| } | ||
| cachedActions = {StyleUtils: createStyleUtils(defaultTheme, cachedState.styles)}; | ||
| } | ||
| return cachedActions[prop]; | ||
| return cachedActions.StyleUtils; |
There was a problem hiding this comment.
src/components/ThemeStylesContextProvider/default.ts L15-31
🟢 The cachedState lazy init is copied into both getters. Can we pull it into one helper so the two can't drift apart ?
function getDefaultState(): ThemeStylesStateContextType {
if (!cachedState) {
cachedState = {styles: styles(defaultTheme)};
}
return cachedState;
}
const defaultThemeStylesStateContextValue: ThemeStylesStateContextType = {
get styles() {
return getDefaultState().styles;
},
};
const defaultThemeStylesActionsContextValue: ThemeStylesActionsContextType = {
get StyleUtils() {
if (!cachedActions) {
cachedActions = {StyleUtils: createStyleUtils(defaultTheme, getDefaultState().styles)};
}
return cachedActions.StyleUtils;
},
};There was a problem hiding this comment.
Extracted the lazy initialization into one private getDefaultState helper used by both getters. Checks confirm that initialization stays lazy and both getters share the same cached objects.
There was a problem hiding this comment.
PR is clean, with 2 small comments ☝️. It removes two as string casts from StatusBadge and replaces the two Proxy-based default contexts with lazy getters. The getters also fix an old problem: spreading the empty-object Proxy used to return {}.
Tag me once comments are addressed.
dylanexpensify
left a comment
dylanexpensify
left a comment
There was a problem hiding this comment.
refactor, product review not required
Seatbelt-Operation: reviewer-fix-100026-3f8e0fc5-ff3c-4842-b116-95ff6d1d7cdb
|
@ikevin127 Could you please check this once you get a chance, Thanks! |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 blimpich has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/blimpich in version: 9.5.15-1 🚀
|
|
No help site update is needed. This PR only removes TypeScript type assertions in status badge and theme style code, so nothing users see or do changes. |
|
🚀 Deployed to production by https://github.com/grgia in version: 9.5.15-3 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
Remove unsafe assertions from
StatusBadgecolor styles and replace the default theme-style context Proxies with explicit lazystylesandStyleUtilsgetters. The getters retain cached values and the existing default theme; badge appearance still needs visual QA.Changed files:
src/components/StatusBadge.tsx,src/components/ThemeStylesContextProvider/default.ts.Fixed Issues
$ #94739
PROPOSAL: #94739 (comment)
Tests
At the current PR head, upstream TypeScript, ESLint, Oxfmt, React Compiler, Storybook tests, and all eight Jest jobs passed.
QA Steps
1. Badge theme changes.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Recordings (1)
pr100026-report-badge-theme-desktop.mp4