[NoQA] Revert "Implementing optimistic violations for workspace categories and tags."#74324
Merged
Conversation
|
@carlosmiceli 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] |
Contributor
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
carlosmiceli
approved these changes
Nov 5, 2025
| ); | ||
| }, | ||
| [policyData, isSetupCategoryTaskParentReportArchived, setupCategoryTaskReport, setupCategoryTaskParentReport, currentUserPersonalDetails.accountID], | ||
| [ |
Contributor
There was a problem hiding this comment.
❌ PERF-6 (docs)
Passing entire objects (policyCategories, policyTagLists, allTransactionViolations) as dependencies causes this useCallback to re-execute whenever ANY property in these objects changes, even unrelated ones. This creates unnecessary re-renders and defeats the purpose of memoization.
Suggested fix: Specify individual properties from these objects that are actually used inside the callback:
const updateWorkspaceCategoryEnabled = useCallback(
(value: boolean, categoryName: string) => {
setWorkspaceCategoryEnabled(
policyId,
{[categoryName]: {name: categoryName, enabled: value}},
isSetupCategoryTaskParentReportArchived,
setupCategoryTaskReport,
setupCategoryTaskParentReport,
currentUserPersonalDetails.accountID,
policyCategories,
policyTagLists,
allTransactionViolations,
);
},
[
policyId,
isSetupCategoryTaskParentReportArchived,
setupCategoryTaskReport,
setupCategoryTaskParentReport,
currentUserPersonalDetails.accountID,
// Instead of passing entire objects, consider refactoring setWorkspaceCategoryEnabled
// to accept policyId and fetch these objects internally, or extract only the specific
// properties needed
],
);| }); | ||
| }, | ||
| [isQuickSettingsFlow, currentCategoryName, policyData, policyID, backTo], | ||
| [currentCategoryName, policyID, policyCategories, isQuickSettingsFlow, backTo], |
Contributor
There was a problem hiding this comment.
❌ PERF-6 (docs)
Passing the entire policyCategories object as a dependency causes this useCallback to re-execute whenever ANY category in the object changes, not just the one being renamed. This creates unnecessary callback recreations.
Suggested fix: Consider refactoring to not depend on the entire policyCategories object:
const handleSubmit = useCallback(
(values: FormOnyxValues<typeof ONYXKEYS.FORMS.WORKSPACE_CATEGORY_FORM>) => {
const newCategoryName = values.categoryName.trim();
if (currentCategoryName !== newCategoryName) {
// Pass policyID and let the action fetch policyCategories internally
renamePolicyCategory(policyID, {oldName: currentCategoryName, newName: values.categoryName}, policyCategories);
}
// ... rest of the code
},
[currentCategoryName, policyID, isQuickSettingsFlow, backTo],
// Removed policyCategories from dependencies
);| setWorkspaceTagEnabled({policyID, tagsToUpdate: {[tagName]: {name: tagName, enabled: value}}, tagListIndex: 0, policyTags}); | ||
| }, | ||
| [policyData], | ||
| [policyID, policyTags], |
Contributor
There was a problem hiding this comment.
❌ PERF-6 (docs)
Passing the entire policyTags object as a dependency causes this useCallback to re-execute whenever ANY tag changes, not just the one being enabled/disabled. This creates unnecessary callback recreations.
Suggested fix: Refactor to avoid depending on the entire policyTags object:
const updateWorkspaceTagEnabled = useCallback(
(value: boolean, tagName: string) => {
// Consider refactoring setWorkspaceTagEnabled to fetch policyTags internally
// or pass only the specific properties needed
setWorkspaceTagEnabled({policyID, tagsToUpdate: {[tagName]: {name: tagName, enabled: value}}, tagListIndex: 0, policyTags});
},
[policyID],
// Removed policyTags from dependencies
);| setPolicyTagsRequired({policyID, requiresTag: value, tagListIndex: orderWeight, policyTags}); | ||
| }, | ||
| [policyData], | ||
| [policyID, policyTags], |
Contributor
There was a problem hiding this comment.
❌ PERF-6 (docs)
Passing the entire policyTags object as a dependency causes this useCallback to re-execute whenever ANY tag changes. This creates unnecessary callback recreations and impacts performance.
Suggested fix: Refactor to avoid depending on the entire policyTags object:
const updateWorkspaceRequiresTag = useCallback(
(value: boolean, orderWeight: number) => {
// Consider refactoring setPolicyTagsRequired to fetch policyTags internally
setPolicyTagsRequired({policyID, requiresTag: value, tagListIndex: orderWeight, policyTags});
},
[policyID],
// Removed policyTags from dependencies
);| }), | ||
| ); | ||
| }, [policyData.policy]); | ||
| }, [currentPolicy]); |
Contributor
There was a problem hiding this comment.
❌ PERF-6 (docs)
Passing the entire currentPolicy object as a dependency causes this useMemo to re-compute whenever ANY property in the policy changes, not just mccGroup. This creates unnecessary recalculations.
Suggested fix: Specify only the mccGroup property as a dependency:
const data = useMemo(() => {
if (!(currentPolicy && currentPolicy.mccGroup)) {
return [];
}
return Object.entries(currentPolicy.mccGroup).map(
([mccKey, mccGroup]): ListItem => ({
categoryID: mccGroup.category,
keyForList: mccKey,
text: mccGroup.category,
pendingAction: mccGroup?.pendingAction,
}),
);
}, [currentPolicy?.mccGroup]);
// Only depend on the specific property being used
Contributor
Author
|
@carlosmiceli Thanks! Waiting to see if the Reassure will complete fine |
iwiznia
approved these changes
Nov 5, 2025
|
@mountiny looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
Contributor
Author
|
Sorry forgot there was approval already 🙈 Merging this as I believe we need to get this to main before we can confirm the reassure tests are fixed as its comparing measurements of main and the branch |
Contributor
Author
|
Straight revert |
54 tasks
79 tasks
Contributor
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.2.45-0 🚀
|
|
@mountiny do we need to QA this in the current regression? |
Contributor
Author
|
No need for QA |
Contributor
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.2.45-6 🚀
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reverts #62627
Testing if this fixes the reassure time outs https://expensify.slack.com/archives/C01GTK53T8Q/p1762352391513639?thread_ts=1762307973.895739&cid=C01GTK53T8Q