81443: Reduce cont of Eslint issues from 399 to 378#81588
81443: Reduce cont of Eslint issues from 399 to 378#81588abbasifaizan70 wants to merge 2 commits intoExpensify:mainfrom
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
melvin-bot
bot
requested review from
brunovjk and
joekaufmanexpensify
and removed request for
a team
|
@neil-marcellini We can mark this as No QA. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const companyCardBankIcons = useCompanyCardBankIcons(); | ||
| const [addNewCard] = useOnyx(ONYXKEYS.ADD_NEW_COMPANY_CARD, {canBeMissing: true}); | ||
| const [typeSelected, setTypeSelected] = useState<CardFeedProvider>(); | ||
| const [localTypeSelected, setLocalTypeSelected] = useState<CardFeedProvider>(); |
There was a problem hiding this comment.
❌ [PERF-6] (docs)
The removed useEffect that syncs addNewCard?.data.feedType to typeSelected state is being replaced with a derived pattern. However, the current implementation doesn't properly derive state - it still maintains typeSelected as separate state instead of computing it directly from props.
Suggested fix: Replace the local state pattern with true state derivation:
const [localTypeSelected, setLocalTypeSelected] = useState<CardFeedProvider>();
const typeSelected = localTypeSelected ?? addNewCard?.data.feedType;This ensures typeSelected is always synchronized with Onyx without needing useEffect.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| return groupTransactionsByCategory(sortedTransactions, report, localeCompare); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [sortedTransactions, currentGroupBy, report?.reportID, localeCompare, shouldShowGroupedTransactions]); | ||
| }, [sortedTransactions, currentGroupBy, report, localeCompare, shouldShowGroupedTransactions]); |
There was a problem hiding this comment.
❌ [PERF-5] (docs)
Including the entire report object in the dependency array causes unnecessary re-renders when any property of report changes. The grouping functions only use report.currency, not the entire report object.
Suggested fix: Use specific field comparison instead of the entire object:
}, [sortedTransactions, currentGroupBy, report?.reportID, report?.currency, localeCompare, shouldShowGroupedTransactions]);This ensures the memoization only breaks when the actually-used properties change.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
![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: 840fde7f94
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const transactionIDSet = useMemo(() => { | ||
| const set = new Set<string>(); | ||
| for (const report of reportsToArchive) { | ||
| if (!report?.iouReportID) { | ||
| continue; | ||
| } | ||
| const reportTransactions = getReportTransactions(report.iouReportID); | ||
| for (const transaction of reportTransactions) { | ||
| set.add(transaction.transactionID); | ||
| } | ||
| } | ||
| } | ||
| return set; | ||
| }, [reportsToArchive]); |
There was a problem hiding this comment.
Because transactionIDSet is now memoized only on reportsToArchive, it won’t update when the transactions for those reports change. If a new transaction (with a violation) is added to an existing report, useOnyx(TRANSACTION_VIOLATIONS) will re-render due to the new violation but transactionIDSet stays stale and filters it out, so the hook can miss violations for newly added transactions. Previously, the set was rebuilt on every render and picked up the latest getReportTransactions data. Consider including a dependency that changes when report transactions update, or avoid memoizing this set.
Useful? React with 👍 / 👎.
|
@abbasifaizan70 Were you able to build it native without problems? I'm getting errors. |
|
No product review needed |
Co-authored-by: Cursor <cursoragent@cursor.com>
@brunovjk I just merged main and then clear old cache built |
3 participants
Explanation of Change
This PR will reduce Lint warnings from 399 to 375
Fixed Issues
$ #81443
PROPOSAL: #81443 (comment)
Tests
No QA.
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 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))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.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari