-
Notifications
You must be signed in to change notification settings - Fork 3.2k
fix: Track seen logins to filter out duplicates in personal details #57459
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Track seen logins to filter out duplicates in personal details #57459
Conversation
All contributors have signed the CLA ✍️ ✅ |
@abdulrahuman5196 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] |
@danstepanov please follow the steps here to sign the CLA |
I have read the CLA Document and I hereby sign the CLA |
@abdulrahuman5196 do you have time to add the requisite videos (presumably just android native?). I am currently busy with other tasks. |
… to handle JSX syntax via the react-jsx compiler option
437fed3
to
6e93955
Compare
1d10e3c
to
da39f89
Compare
I have some additional changes I'd like to make to this that would further improve performance but, in the interest of moving onto other tasks, this does solve the problem. |
Thats sounds good, we usually create a follow up issue ticket for that! |
tsconfig.json
Outdated
@@ -22,7 +22,8 @@ | |||
"@styles/*": ["./src/styles/*"], | |||
"@src/*": ["./src/*"], | |||
"@userActions/*": ["./src/libs/actions/*"] | |||
} | |||
}, | |||
"jsx": "react-jsx" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does this exactly do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure why I didn't get these notifications but I made this change for reasons unrelated directly to this PR, I can remove it. I was running into Typescript issues while working on the issue and this seemed to resolve them. This has to do with how JSX is transformed in TS files.
tsconfig.json
Outdated
@@ -22,7 +22,8 @@ | |||
"@styles/*": ["./src/styles/*"], | |||
"@src/*": ["./src/*"], | |||
"@userActions/*": ["./src/libs/actions/*"] | |||
} | |||
}, | |||
"jsx": "react-jsx" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change seems unrelated to this pr, can you please remove it?
tsconfig.json
Outdated
@@ -22,7 +22,8 @@ | |||
"@styles/*": ["./src/styles/*"], | |||
"@src/*": ["./src/*"], | |||
"@userActions/*": ["./src/libs/actions/*"] | |||
} | |||
}, | |||
"jsx": "react-jsx" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be needed here. Please remove it as said above ^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@danstepanov Bump on this one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry for the delay here, I'm just seeing these messages. I've removed the change.
Thanks. I will start on this today |
One redundant change, the rest looks fine. |
@danstepanov Ops.... While testing on some chats, I see that this problem still happens on IOS Screen.Recording.2025-03-03.at.16.55.10.mov |
From my investigation, the suggestion list doesn't display because of this check App/src/components/AutoCompleteSuggestions/index.tsx Lines 142 to 144 in 9b85822
From my log, I see that:
|
Co-authored-by: DylanDylann <[email protected]>
…y fallback values, remove irrelevant comment
8f38734
to
0c00b31
Compare
@danstepanov I think we also need to revert these changes ![]() |
}); | ||
}); | ||
}, [measureParentContainerAndReportCursor, windowHeight, windowWidth, keyboardHeight, shouldUseNarrowLayout, suggestionsLength, bottomInset, topInset, isKeyboardAnimatingRef]); | ||
|
||
if ((containerState.width === 0 && containerState.left === 0 && containerState.bottom === 0) || (containerState.cursorCoordinates.x === 0 && containerState.cursorCoordinates.y === 0)) { | ||
// Only prevent rendering if we have no suggestions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment doesn't reflect all below conditions
…s to isEnoughSpaceToRenderMenuAboveForSmall
…ng the AutoCompleteSuggestionsPortal
@@ -134,14 +146,16 @@ function AutoCompleteSuggestions<TSuggestion>({measureParentContainerAndReportCu | |||
left: leftValue.current, | |||
bottom: bottomValue, | |||
width: widthValue, | |||
cursorCoordinates, | |||
cursorCoordinates: {x: cursorCoordinates.x, y: cursorCoordinates.y}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@danstepanov One more place, please revert it
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2025-03-11.at.16.58.36.movAndroid: mWeb ChromeScreen.Recording.2025-03-11.at.17.01.15.moviOS: NativeScreen.Recording.2025-03-11.at.16.59.17.moviOS: mWeb SafariScreen.Recording.2025-03-11.at.17.00.17.movMacOS: Chrome / SafariScreen.Recording.2025-03-11.at.17.02.38.movMacOS: DesktopScreen.Recording.2025-03-11.at.17.01.58.mov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@danstepanov please address comment here to make things consistent. Other than that LGTM
Co-authored-by: DylanDylann <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. @mountiny if you want to check it
All good, not reviewing but going to merge as you approved |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/mountiny in version: 9.1.13-0 🚀
|
@danstepanov @DylanDylann Can you please follow up, seems like its not fixed thanks! |
@kavimuru This appears to be inconsistent across accounts. If possible, could you export your Onyx data? That way, we can take a deeper look into it and assist you further. |
🚀 Deployed to production by https://github.com/mountiny in version: 9.1.13-5 🚀
|
Explanation of Change
Optimized the filtering of personal details in mention suggestions by using a Set to track duplicate logins instead of using array.findIndex. This visibly improves performance and fixes flickering issues when typing @ in the composer, particularly noticeable with large numbers of personal details. This issue only pertains to iOS.
Fixed Issues
$ #54991
PROPOSAL: #54991 (comment)
Tests
Offline tests
No offline testing needed as this is a UI performance optimization.
QA Steps
Sam as tests
PR Author Checklist
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
).src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
have been tested & I retested again)/** comment above it */
this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Before
https://github.com/user-attachments/assets/741eef7a-504d-4cae-8c6c-382df043d6da
After
https://github.com/user-attachments/assets/bf633fc7-f211-4686-9852-9640ae10f47e
Android: mWeb Chrome
Before
https://github.com/user-attachments/assets/741eef7a-504d-4cae-8c6c-382df043d6da
After
https://github.com/user-attachments/assets/bf633fc7-f211-4686-9852-9640ae10f47e
iOS: Native
Before
https://github.com/user-attachments/assets/741eef7a-504d-4cae-8c6c-382df043d6da
After
https://github.com/user-attachments/assets/bf633fc7-f211-4686-9852-9640ae10f47e
iOS: mWeb Safari
Before
https://github.com/user-attachments/assets/741eef7a-504d-4cae-8c6c-382df043d6da
After
https://github.com/user-attachments/assets/bf633fc7-f211-4686-9852-9640ae10f47e
MacOS: Chrome / Safari
Before
https://github.com/user-attachments/assets/741eef7a-504d-4cae-8c6c-382df043d6da
After
https://github.com/user-attachments/assets/bf633fc7-f211-4686-9852-9640ae10f47e
MacOS: Desktop
Before
https://github.com/user-attachments/assets/741eef7a-504d-4cae-8c6c-382df043d6da
After
https://github.com/user-attachments/assets/bf633fc7-f211-4686-9852-9640ae10f47e