[HOLD] Patch for getting connection IDs by key #36226

gedu · 2024-02-09T13:28:37Z

Details

Implemented some improvements on the onyx side, all in the details in the issue ticket

Fixed Issues

$ #35809
PROPOSAL: -

Tests

Testing, will be mainly use the app, to know that my changes doesn't break anything specially collections

Verify that no errors appear in the JS console

Offline tests

QA Steps

Verify that no errors appear in the JS console

PR Author Checklist

I linked the correct issue in the ### Fixed Issues section above
I wrote clear testing steps that cover the changes made in this PR
- I added steps for local testing in the Tests section
- I added steps for the expected offline behavior in the Offline steps section
- I added steps for Staging and/or Production testing in the QA steps section
- I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
- I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
I included screenshots or videos for tests on all platforms
I ran the tests on all platforms & verified they passed on:
- Android: Native
- Android: mWeb Chrome
- iOS: Native
- iOS: mWeb Safari
- MacOS: Chrome / Safari
- MacOS: Desktop
I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
I followed proper code patterns (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
  - If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I followed the guidelines as stated in the Review Guidelines
I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
I verified that if a function's arguments changed that all usages have also been updated correctly
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If the PR modifies the form input styles:
- I verified that all the inputs inside a form are aligned with each other.
- I added Design label so the design team can review the changes.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

MacOS: Desktop

melvin-bot · 2024-02-09T13:28:42Z

@akinwale 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]

mollfpr · 2024-02-09T13:38:02Z

Sorry @akinwale, I'll review this 🙏

mollfpr · 2024-02-09T18:09:07Z

patches/react-native-onyx+2.0.1.patch

+    const keyToDelete = subscriber.key;
+
+    if (onyxKeyToConnectionIDs.has(keyToDelete)) {
+        onyxKeyToConnectionIDs.set(keyToDelete, _.without(onyxKeyToConnectionIDs.get(keyToDelete), connectionID));


Failing perf test.

I'm chatting with the reassure devs to fix this

mollfpr · 2024-02-12T13:15:57Z

I locally pulled the latest main to the branch and fixed the issue with the _ is not defined locally. Also, run npm run clean and run the app with reset-cache.

All the tests below are tested on the Web, on the native platform it's very laggy. It might be because the console.log that's showing for a hundred thousand. Also the issues I found already check on the main branch, and it's not reproducible.

Settings
✅ Profile
✅ Share code
✅ Preference

Report
✅ Start a chat
✅ Send a message
✅ Right-click to open the context menu
❌ React to a message
❌ Reply in the thread
❌ Edit a message
✅ Copy to clipboard
✅ Delete message
✅ Mark as unread
✅ Open the emoji picker from the context menu
✅ Open an attachment

Chat composer
✅ Send a message
✅ Send a text with an emoji
✅ Send a text with a markdown
❌ Edit message with up arrow key
✅ Press the actions button
✅ Open the emoji picker
✅ Mention

✅ Assign task

Request money
✅ Manual request
✅ Scan request
✅ Distance request

Split bill
✅ Manual request
✅ Scan request
✅ Distance request

✅ Send money

✅ Add attachment

Workspace
✅ Workspace switcher
✅ Create a new workspace
✅ Workspace profile
✅ Add/remove members

Issues

Sometimes can’t open an IOU report and task report. The report keeps loading and request to API OpenReport.
High memory usage on Web Chrome
The App does not send the API request after use for a while and works after refreshing the page.
The message that I try edit previously, suddenly opens the edit message composer and it won’t close. Clicking on the report main composer move the cursor to the edit message composer.
The LHN doesn’t change the latest message text after sending a message, need to refresh to correctly see the latest message on LHN.
Send multiple requests of money in a row (manual then scan receipt), the IOU preview doesn’t show the scanned receipt thumbnail.

cc @gedu @mountiny

gedu · 2024-02-12T17:24:05Z

@mollfpr Cool great testing coverage.
Saw what it is going on, it is sending as a key eg: reportActionsDraft_123344, so it doesn't find that key, because is never saved as connect, I need to do the same extraction that it is done in other part of the code, will add a fix and update the patch

gedu · 2024-02-12T17:53:17Z

Will do a round of tests but it is working now

fixing_collections_patch.mp4

mountiny · 2024-02-14T14:42:22Z

@mollfpr really great testing, thank you

@gedu let us know when this is ready for another look

gedu · 2024-02-14T18:10:39Z

@mountiny the TS is failing because the App is using onyx v.2.0.2 instead of v2.0.4, I pushed the missing type into the patch so this works, I can remove it later, should be ready for testing

mountiny · 2024-02-14T21:59:55Z

@mollfpr Will you be able to give this another round of testing please?

mollfpr · 2024-02-15T17:26:10Z

@mountiny Yup, running another test!

roryabraham · 2024-02-15T19:11:07Z

@gedu why are you using a patch file to test Onyx? You can test Onyx locally w/ live-reload using the technique described in the README here. PRs can be opened in Onyx and tested by C+ and reviewers in the same way, without making it difficult to read and review the diff.

I suppose the ability to run AdHoc builds is nice, but this PR should not be merged, so imo it should be marked as a draft or POC

gedu · 2024-02-16T09:46:41Z

@gedu why are you using a patch file to test Onyx? You can test Onyx locally w/ live-reload using the technique described in the README here. PRs can be opened in Onyx and tested by C+ and reviewers in the same way, without making it difficult to read and review the diff.

I suppose the ability to run AdHoc builds is nice, but this PR should not be merged, so imo it should be marked as a draft or POC

I created the patch because we were testing with a release build and using the Android device, so the live-reload wouldn't work.
I asked the best way to handle this case if should I create a new PR in Onyx etc. I can make it a draft PR and if this works create a PR for Onyx

mountiny · 2024-02-16T14:28:28Z

Yeah we know its not supposed to be merged, but this works well for testing for any regressions, added HOLD so nobody accidentally merges it

@mollfpr have you been able to conclude another round of testing please?

mollfpr · 2024-02-16T19:13:09Z

Sorry for the delay, the test I did previously now PASS on the web and desktop. For the native platform, I can't run it locally.

mountiny · 2024-02-19T18:29:00Z

@gedu do you ahve any tips for the native platforms?

github-actions · 2024-02-19T18:46:55Z

🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪

Android 🤖	iOS 🍎
https://ad-hoc-expensify-cash.s3.amazonaws.com/android/36226/index.html	https://ad-hoc-expensify-cash.s3.amazonaws.com/ios/36226/index.html

Desktop 💻	Web 🕸️
https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/36226/NewExpensify.dmg	https://36226.pr-testing.expensify.com

👀 View the workflow run that generated this build 👀

mountiny · 2024-02-19T19:26:14Z

@mollfpr can you test these builds?

mountiny · 2024-02-19T19:47:30Z

The ios build des not work for me so I assume the Android wont work either

gedu · 2024-02-20T07:25:14Z

Hey, will check native, it should work we did all the tests on Android

gedu · 2024-02-20T14:25:47Z

Locally I can run it, and it works.
Is it possible that we only send as key '123212' or '' ?

mountiny · 2024-02-20T19:08:12Z

@mollfpr can you test this locally?

mollfpr · 2024-02-21T12:48:43Z

I test the latest Android ad-hoc and build locally on the physical device and emulator. None of them work for me. It's only showing the splash screen. I also, run the npm run clean before building it locally.

mollfpr · 2024-02-21T13:02:20Z

Still shows a blank screen on the iOS build.

gedu · 2024-02-21T13:44:48Z

Should I do a PR in the Onyx repository so you can test it with the watch?

gedu · 2024-03-26T11:44:05Z

@mountiny Created a PR at onyx repository: Expensify/react-native-onyx#520

mountiny · 2024-03-29T23:13:39Z

Thanks, that will be on hold until we move ahead with onyx bump

gedu · 2024-07-05T05:56:21Z

@mountiny Not sure how to handle this one, but we can close it in favor of: Expensify/react-native-onyx#565

mountiny · 2024-07-05T09:06:46Z

Yep, lets close this now, thank you

Patch for getting connection IDs by key

0d5b501

gedu requested a review from a team as a code owner February 9, 2024 13:28

melvin-bot bot removed the request for review from a team February 9, 2024 13:28

melvin-bot bot requested a review from akinwale February 9, 2024 13:28

gedu mentioned this pull request Feb 9, 2024

Onyx: keyChanged improvement - patch #36008

Closed

50 tasks

mollfpr reviewed Feb 9, 2024

View reviewed changes

mountiny removed the request for review from akinwale February 10, 2024 15:55

mountiny assigned gedu Feb 10, 2024

mountiny added the Ready To Build label Feb 10, 2024

This comment has been minimized.

Sign in to view

Merge branch 'main' into gedu/onyx_key_changed_new_approach_patch

8812cf3

gedu added 2 commits February 14, 2024 15:46

Merge branch 'main' into gedu/onyx_key_changed_new_approach_patch

7363ee6

Using the collection key to get the connection IDs

8c38896

adhorodyski mentioned this pull request Feb 14, 2024

[Tracking] Metric - Report screen load time (Performance audit by Callstack) #34494

Closed

gedu added 2 commits February 14, 2024 17:20

Merge branch 'main' into gedu/onyx_key_changed_new_approach_patch

2f493aa

Merge branch 'main' into gedu/onyx_key_changed_new_approach_patch

9f2fd2d

mountiny changed the title ~~Patch for getting connection IDs by key~~ [HOLD] Patch for getting connection IDs by key Feb 16, 2024

gedu mentioned this pull request Mar 26, 2024

keyChange update key/connectionID for faster lookup Expensify/react-native-onyx#520

Closed

42 tasks

mountiny closed this Jul 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HOLD] Patch for getting connection IDs by key #36226

[HOLD] Patch for getting connection IDs by key #36226

gedu commented Feb 9, 2024

melvin-bot bot commented Feb 9, 2024

mollfpr commented Feb 9, 2024

mollfpr Feb 9, 2024

gedu Feb 12, 2024

This comment has been minimized.

mollfpr commented Feb 12, 2024

gedu commented Feb 12, 2024

gedu commented Feb 12, 2024

mountiny commented Feb 14, 2024

gedu commented Feb 14, 2024

mountiny commented Feb 14, 2024

mollfpr commented Feb 15, 2024

roryabraham commented Feb 15, 2024

gedu commented Feb 16, 2024

mountiny commented Feb 16, 2024

mollfpr commented Feb 16, 2024 •

edited

Loading

mountiny commented Feb 19, 2024

github-actions bot commented Feb 19, 2024

mountiny commented Feb 19, 2024

mountiny commented Feb 19, 2024

gedu commented Feb 20, 2024

gedu commented Feb 20, 2024

mountiny commented Feb 20, 2024

mollfpr commented Feb 21, 2024

mollfpr commented Feb 21, 2024

gedu commented Feb 21, 2024

gedu commented Mar 26, 2024

mountiny commented Mar 29, 2024

gedu commented Jul 5, 2024

mountiny commented Jul 5, 2024

[HOLD] Patch for getting connection IDs by key #36226

[HOLD] Patch for getting connection IDs by key #36226

Conversation

gedu commented Feb 9, 2024

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Feb 9, 2024

mollfpr commented Feb 9, 2024

mollfpr Feb 9, 2024

Choose a reason for hiding this comment

gedu Feb 12, 2024

Choose a reason for hiding this comment

This comment has been minimized.

mollfpr commented Feb 12, 2024

gedu commented Feb 12, 2024

gedu commented Feb 12, 2024

mountiny commented Feb 14, 2024

gedu commented Feb 14, 2024

mountiny commented Feb 14, 2024

mollfpr commented Feb 15, 2024

roryabraham commented Feb 15, 2024

gedu commented Feb 16, 2024

mountiny commented Feb 16, 2024

mollfpr commented Feb 16, 2024 • edited Loading

mountiny commented Feb 19, 2024

github-actions bot commented Feb 19, 2024

mountiny commented Feb 19, 2024

mountiny commented Feb 19, 2024

gedu commented Feb 20, 2024

gedu commented Feb 20, 2024

mountiny commented Feb 20, 2024

mollfpr commented Feb 21, 2024

mollfpr commented Feb 21, 2024

gedu commented Feb 21, 2024

gedu commented Mar 26, 2024

mountiny commented Mar 29, 2024

gedu commented Jul 5, 2024

mountiny commented Jul 5, 2024

mollfpr commented Feb 16, 2024 •

edited

Loading