Conversation
melvin-bot
bot
requested review from
joekaufmanexpensify
and removed request for
a team
|
🚧 @dubielzyk-expensify 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! 🧪🧪
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| import React from 'react'; | ||
| import {LogBox, View} from 'react-native'; | ||
| import React, {useCallback, useEffect, useState} from 'react'; | ||
| // eslint-disable-next-line no-restricted-imports |
There was a problem hiding this comment.
❌ CONSISTENCY-5 (docs)
The eslint-disable-next-line no-restricted-imports comment lacks a justification explaining why the restricted import is necessary here. ESLint rule disables without justification can mask underlying issues and reduce code quality.
Add a justification comment:
// eslint-disable-next-line no-restricted-imports -- Need raw RN useWindowDimensions to get true viewport width before EffectiveWidthContext override
import {LogBox, useWindowDimensions as useRawWindowDimensions, View} from 'react-native';Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| right: FLOATING_PANEL_MARGIN, | ||
| width: FLOATING_PANEL_WIDTH, | ||
| height: FLOATING_PANEL_HEIGHT, | ||
| borderRadius: 12, |
There was a problem hiding this comment.
❌ CONSISTENCY-2 (docs)
The floating panel inline style contains multiple magic values: borderRadius: 12, shadowColor: '#000', shadowOffset: {width: 0, height: 8}, shadowOpacity: 0.15, shadowRadius: 24. These hardcoded numbers and the color string bypass the theme system and reduce maintainability.
Extract the floating panel style to useThemeStyles where it can use theme variables and be reused:
// In the styles generator:
floatingInboxPanel: {
borderRadius: variables.componentBorderRadius,
overflow: 'hidden',
borderWidth: 1,
borderColor: theme.border,
...shadow(theme),
}Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| onPress={closePanel} | ||
| role={CONST.ROLE.BUTTON} | ||
| accessibilityLabel={translate('common.close')} | ||
| style={[styles.touchableButtonImage, {marginLeft: -10}]} |
There was a problem hiding this comment.
❌ CONSISTENCY-2 (docs)
{marginLeft: -10} is a magic number inline style with no documentation explaining its purpose. The negative margin is particularly opaque without context.
Extract to a named constant with a comment, or define in useThemeStyles:
// If this offsets the button to align with content above:
const CLOSE_BUTTON_ALIGNMENT_OFFSET = -10;Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| import CONST from '@src/CONST'; | ||
| import {useInboxPanelActions, useInboxPanelState} from './InboxPanelContext'; | ||
|
|
||
| type InboxStackParamList = { |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
The InboxStackParamList type is defined identically in three files: InboxListScreen.tsx (line 18), InboxReportScreen.tsx (line 13), and index.tsx (line 10). Any change to the param list requires updating all three locations.
Define the type once and import it everywhere:
// src/components/InboxSidePanel/types.ts
export type InboxStackParamList = {
InboxList: undefined;
InboxReport: {reportID: string};
};Then import from './types' in each file.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| const {isOpen, isFloating} = useInboxPanelState(); | ||
| const {width: rawWindowWidth} = useRawWindowDimensions(); | ||
| const theme = useTheme(); | ||
| const panelWidth = Math.max(rawWindowWidth * 0.2, 350); |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
The panel width formula Math.max(rawWindowWidth * 0.2, 350) is duplicated in three places: App.tsx line 91, App.tsx line 112 (inside useEffect), and useRootNavigatorScreenOptions.ts line 53. If the sizing logic changes, all three must be updated.
Extract to a shared utility:
const MIN_INBOX_PANEL_WIDTH = 350;
const INBOX_PANEL_WIDTH_RATIO = 0.2;
export function getInboxPanelWidth(viewportWidth: number): number {
return Math.max(viewportWidth * INBOX_PANEL_WIDTH_RATIO, MIN_INBOX_PANEL_WIDTH);
}Import and call getInboxPanelWidth(rawWindowWidth) in all three locations.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| // Measured width of the main content container, provided to all children via | ||
| // EffectiveWidthContext so useWindowDimensions returns the true available width. | ||
| const [mainContentWidth, setMainContentWidth] = useState(rawWindowWidth); | ||
| const onMainContentLayout = useCallback((e: LayoutChangeEvent) => { |
There was a problem hiding this comment.
❌ CLEAN-REACT-PATTERNS-0 (docs)
React Compiler is enabled and this file compiles successfully. The useCallback wrapping onMainContentLayout is redundant -- the compiler automatically memoizes closures based on their captured variables.
Remove the useCallback wrapper:
const onMainContentLayout = (e: LayoutChangeEvent) => {
setMainContentWidth(e.nativeEvent.layout.width);
};Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| }); | ||
| }, [navigation, registerPanelNavigation]); | ||
|
|
||
| const onSelectRow = useCallback( |
There was a problem hiding this comment.
❌ CLEAN-REACT-PATTERNS-0 (docs)
React Compiler is enabled and this file compiles successfully. The useCallback wrapping onSelectRow is redundant -- the compiler automatically memoizes closures based on their captured variables.
Remove the useCallback wrapper:
const onSelectRow = (option: OptionData) => {
if (!option.reportID) {
return;
}
navigation.navigate('InboxReport', {reportID: option.reportID});
};Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| function InboxReportScreen({navigation}: {navigation: ReportScreenProps['navigation']}) { | ||
| const localNavigation = useNavigation<StackNavigationProp<InboxStackParamList>>(); | ||
| const route = useRoute<RouteProp<InboxStackParamList, 'InboxReport'>>(); | ||
|
|
There was a problem hiding this comment.
❌ CLEAN-REACT-PATTERNS-0 (docs)
React Compiler is enabled and this file compiles successfully. The useCallback for goBack (line 22) and useMemo for sidePanelActions (line 24) are both redundant -- the compiler automatically memoizes closures and derived values.
Remove the manual memoization:
const goBack = () => localNavigation.goBack();
const sidePanelActions = {
openSidePanel: () => {},
closeSidePanel: goBack,
};Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| setIsOpen(true); | ||
| if (panelNavigateRef.current) { | ||
| panelNavigateRef.current(reportID); | ||
| } else { |
There was a problem hiding this comment.
❌ CLEAN-REACT-PATTERNS-0 (docs)
React Compiler is enabled and this file compiles successfully. Multiple useCallback calls (lines 54-73: openPanel, closePanel, togglePanel, toggleFloating, registerPanelNavigation, navigateToReport) and useMemo calls (lines 75-79: stateValue, actionsValue) are all redundant -- the compiler automatically memoizes closures and derived values.
Remove all manual memoization wrappers:
const openPanel = () => setIsOpen(true);
const closePanel = () => setIsOpen(false);
const togglePanel = () => setIsOpen((prev) => !prev);
const toggleFloating = () => setIsFloating((prev) => !prev);
// ... similarly for registerPanelNavigation, navigateToReport
const stateValue = {isOpen, isFloating};
const actionsValue = {openPanel, closePanel, togglePanel, toggleFloating, registerPanelNavigation, navigateToReport};Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
1 participant
Explanation of Change
This is a prototype that moves the Inbox tab from a navigational element to a docked or floating panel on the right. The aim is to give the ability to access any chat from anywhere in the app.
This is just a proof of concept.
Fixed Issues
$
PROPOSAL:
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, 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.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./** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari