Skip to content
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

[PAID 1/7/25] [$250] Web - Search - New messages are not updated automatically in chats section #52193

Closed
1 of 8 tasks
lanitochka17 opened this issue Nov 7, 2024 · 48 comments
Closed
1 of 8 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@lanitochka17
Copy link

lanitochka17 commented Nov 7, 2024

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: 9.0.59-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: N/A
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/5192577&group_by=cases:section_id&group_order=asc&group_id=296775
Issue reported by: Applause - Internal Team

Action Performed:

  1. Open the staging.new.expensify.com website
  2. Click on "Search" on the bottom of the screen
  3. Click on "Chats"
  4. From another browser or device, send a message to the main account
  5. Verify the new message is automatically updated on the chat section

Expected Result:

New messages should be updated in real time on chats section on main device, as it happens with new expenses or deleted messages

Actual Result:

New messages on chats section are not displayed until navigating out of the section and returning. Behaviour with deleted messages or new expenses is different, as they are displayed in real time

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android: Standalone
  • Android: HybridApp
  • Android: mWeb Chrome
  • iOS: Standalone
  • iOS: HybridApp
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence
Bug6657705_1730991181627.Search_Chats.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021856702046390987958
  • Upwork Job ID: 1856702046390987958
  • Last Price Increase: 2024-11-20
  • Automatic offers:
    • FitseTLT | Contributor | 105128912
Issue OwnerCurrent Issue Owner: @mananjadhav
@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Nov 7, 2024
Copy link

melvin-bot bot commented Nov 7, 2024

Triggered auto assignment to @CortneyOfstad (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@melvin-bot melvin-bot bot added the Overdue label Nov 11, 2024
Copy link

melvin-bot bot commented Nov 13, 2024

@CortneyOfstad Huh... This is 4 days overdue. Who can take care of this?

@CortneyOfstad
Copy link
Contributor

Was able to consistently recreate this between my [email protected] and [email protected] accounts.

The only way I could get the messages to appear was a forced refresh or actively clicking out and back into the chats page.

Going to get eyes on this!

@melvin-bot melvin-bot bot removed the Overdue label Nov 13, 2024
@CortneyOfstad CortneyOfstad added External Added to denote the issue can be worked on by a contributor Overdue labels Nov 13, 2024
@melvin-bot melvin-bot bot changed the title Web - Search - New messages are not updated automatically in chats section [$250] Web - Search - New messages are not updated automatically in chats section Nov 13, 2024
Copy link

melvin-bot bot commented Nov 13, 2024

Job added to Upwork: https://www.upwork.com/jobs/~021856702046390987958

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 13, 2024
Copy link

melvin-bot bot commented Nov 13, 2024

Triggered auto assignment to Contributor-plus team member for initial proposal review - @mananjadhav (External)

@FitseTLT
Copy link
Contributor

FitseTLT commented Nov 16, 2024

Edited by proposal-police: This proposal was edited at 2024-11-22 18:50:08 UTC.

Proposal

Please re-state the problem that we are trying to solve in this issue.

Web - Search - New messages are not updated automatically in chats section

What is the root cause of that problem?

New Feature

What changes do you think we should make in order to solve the problem?

We can update useSearchHighlightAndScroll which is handling only the searchHighlight of expenses to start supporting chat too. We will update it to handle report action highlighting for chat case and transaction highlighting for expenses

  1. We will listen to reportActions_ key and hold prevReportActions value to compare the new report action added
 const [reportActions] = useOnyx(ONYXKEYS.COLLECTION.REPORT_ACTIONS);
    const previousReportActions = usePrevious(reportActions);

and pass it to the hook here

const {newSearchResultKey, handleSelectionListScroll} = useSearchHighlightAndScroll({
searchResults,
transactions,
previousTransactions,
queryJSON,
offset,


        reportActions,
        previousReportActions,
  1. inside the hook we will trigger a search action when there is new report action added
    if (transactionsLength && typeof previousTransactionsLength === 'number' && transactionsLength > previousTransactionsLength) {
    // Set the flag indicating the search is triggered by the hook
    triggeredByHookRef.current = true;
    // Trigger the search
    SearchActions.search({queryJSON, offset});
        const reportActionsLength = reportActions && Object.values(reportActions).reduce((sum, curr) => sum + Object.keys(curr).length, 0);
        const prevReportActionsLength = previousReportActions && Object.values(previousReportActions).reduce((sum, curr) => sum + Object.keys(curr).length, 0);
        // Return early if search was already triggered or there's no change in transactions length
        if (searchTriggeredRef.current || (isExpense && previousTransactionsLength === transactionsLength) || (isChat && reportActionsLength === prevReportActionsLength)) {
            return;
        }
        const newTransactionAdded = transactionsLength && typeof previousTransactionsLength === 'number' && transactionsLength > previousTransactionsLength;

        const newReportActionAdded = typeof reportActionsLength === 'number' && typeof prevReportActionsLength === 'number' && reportActionsLength > prevReportActionsLength;
        // Check if a new transaction was added
        if ((!isChat && newTransactionAdded) || (isChat && newReportActionAdded)) {
            // Set the flag indicating the search is triggered by the hook
           
  1. We will identify the id of the new report action add and set newSearchResultKey
    const newTransactionKey = `${ONYXKEYS.COLLECTION.TRANSACTION}${newTransactionID}`;
    setNewSearchResultKey(newTransactionKey);
    highlightedTransactionIDs.current.add(newTransactionID);
  if (!isChat) {
            const previousTransactionIDs = extractTransactionIDsFromSearchResults(previousSearchResults);
            const currentTransactionIDs = extractTransactionIDsFromSearchResults(searchResults.data);

            // Find new transaction IDs that are not in the previousTransactionIDs and not already highlighted
            const newTransactionIDs = currentTransactionIDs.filter((id) => !previousTransactionIDs.includes(id) && !highlightedIDs.current.has(id));

            if (!triggeredByHookRef.current || newTransactionIDs.length === 0) {
                return;
            }

            const newTransactionID = newTransactionIDs.at(0) ?? '';
            const newTransactionKey = `${ONYXKEYS.COLLECTION.TRANSACTION}${newTransactionID}`;

            setNewSearchResultKey(newTransactionKey);
            highlightedIDs.current.add(newTransactionID);
        }
        if (isChat) {
            const previousReportActionIDs = extractReportActionIDsFromSearchResults(previousSearchResults);
            const currentReportActionIDs = extractReportActionIDsFromSearchResults(searchResults.data);

            // Find new transaction IDs that are not in the previousTransactionIDs and not already highlighted
            const newReportActionIDs = currentReportActionIDs.filter((id) => !previousReportActionIDs.includes(id) && !highlightedIDs.current.has(id));

            if (!triggeredByHookRef.current || newReportActionIDs.length === 0) {
                return;
            }

            const newReportActionID = newReportActionIDs.at(0) ?? '';
            const newReportActionKey = `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${newReportActionID}`;

            setNewSearchResultKey(newReportActionKey);
            highlightedIDs.current.add(newReportActionID);
        }
    }, [searchResults?.data, previousSearchResults, isExpense, isChat]);

  1. Then add the necessary styling in ChatItem .
    And also other minor changes I think a Test branch is better approach here.

POC:

2024-11-16.03-28-00.mp4

What alternative solutions did you explore? (Optional)

@melvin-bot melvin-bot bot added the Overdue label Nov 16, 2024
@huult
Copy link
Contributor

huult commented Nov 16, 2024

Proposal

Please re-state the problem that we are trying to solve in this issue.

New messages are not updated automatically in chats section

What is the root cause of that problem?

This is a new feature of react-native-onyx.

When User A sends a message to User B, User B will receive new data through the Pusher event.

function subscribeToPrivateUserChannelEvent(eventName: string, accountID: string, onEvent: (pushJSON: OnyxUpdatesFromServer) => void) {

And User B listens to this event to receive new data from User A and stores it in Onyx.

In this new data included new message from User A sent we have update it to reportAction_ and already called to update to snapshot_ but this value key not exisitng in snapshot_ so we can't update it.

This new data includes a new message sent by User A, which we have updated in reportAction_ and already called to update in snapshot_. However, the value key does not exist in snapshot_, so we can't update it. The reason it doesn't exist is that this is a new message.

https://github.com/Expensify/react-native-onyx/blob/11407f663b605591acfe7e5d5a4265466b7f61ce/lib/Onyx.ts#L576-L579

const newValue = lodashPick(value, Object.keys(snapshotData[key]));

As per this code, we only have a new value when the value key exists, and then we are able to pick it.

To debug this, we can add the log following the path node_modules/react-native-onyx/dist/Onyx.js. and go to updateSnapshots function and add this code under let newValue = (0, pick_1.default)(value, Object.keys(snapshotData[key])); to see what i mentioned

            console.log('****** snapshotData[key] ******', snapshotData[key]);
            console.log('****** value key ******', Object.keys(value)[0]);
            if (snapshotData[key] && Object.keys(snapshotData[key]).length > 0) {
                const firstValueKey = Object.keys(value)[0];
                const snapshotKeys = Object.keys(snapshotData[key]);

                if (!snapshotKeys.includes(firstValueKey)) {
                    console.log(`****** ${firstValueKey} key not exisitng in ${key} ******`, );
                }
            }

So, because we can't add new data to snapshot_, the message will not be displayed when User A sends a new message.

VIDEO: Debug this issue.

What changes do you think we should make in order to solve the problem?

To resolve this issue, we must update react-native-onyx to allow adding new values to snapshot_ if the value key does not exist in snapshot_. The code change will be:

  1. Create a new function to add a new value to snapshot_.
// node_modules/react-native-onyx/dist/Onyx.js#L472
function addSnapshots(data) {
    const snapshotCollectionKey = OnyxUtils_1.default.getSnapshotKey();
    if (!snapshotCollectionKey)
        return;
    const promises = [];
    const snapshotCollection = OnyxUtils_1.default.getCachedCollection(snapshotCollectionKey);
    const snapshotCollectionKeyLength = snapshotCollectionKey.length;

    Object.entries(snapshotCollection).forEach(([snapshotKey, snapshotValue]) => {
        // Snapshots may not be present in cache. We don't know how to update them so we skip.
        if (!snapshotValue) {
            return;
        }
        let addedData = {};
        data.forEach(({ key, value }) => {
            // snapshots are normal keys so we want to skip update if they are written to Onyx
            if (OnyxUtils_1.default.isCollectionMemberKey(snapshotCollectionKey, key, snapshotCollectionKeyLength)) {
                return;
            }
            if (typeof snapshotValue !== 'object' || !('data' in snapshotValue)) {
                return;
            }
            const snapshotData = snapshotValue.data;
            if (!snapshotData || !snapshotData[key]) {
                return;
            }

            let newValue = {};

            // Check if snapshotData contains the key and has values
            if (snapshotData[key] && Object.keys(snapshotData[key]).length > 0) {
                const firstValueKey = Object.keys(value)[0];
                const snapshotKeys = Object.keys(snapshotData[key]);

                // Check if the first key in `value` doesn't exist in `snapshotData[key]`
                if (!snapshotKeys.includes(firstValueKey)) {
                    const existingValue = Object.values(snapshotData[key])[0];
                    const newValues = Object.values(value)[0];

                    // Update `existingValue` with matching keys from `newValues`
                    Object.keys(newValues).forEach(subKey => {
                        if (subKey in existingValue) {
                            existingValue[subKey] = newValues[subKey];
                        }
                    });

                    newValue = existingValue;
                }
            }

            addedData = {
                ...addedData,
                [key]: newValue,
            };

        });
        // Skip the update if there's no data to be merged
        if (utils_1.default.isEmptyObject(addedData)) {
            return;
        }
       
        
        promises.push(() => merge(snapshotKey, { data: addedData }));
    });
    return Promise.all(promises.map((p) => p()));
}
  1. Call it inside Onyx Update
// node_modules/react-native-onyx/dist/Onyx.js#L675
    return clearPromise
        .then(() => Promise.all(promises.map((p) => p())))
+        .then(() => addSnapshots(data))
        .then(() => updateSnapshots(data))
        .then(() => undefined);
  1. If we need to highlight and scroll to the message, we will use useSearchHighlightAndScroll and create currentReportAction and previousReportAction to identify the new item and scroll to it. We will discuss this behavior in the pull request if needed.

To test my proposal, you can download the file Onyx.js and replace the Onyx.js file in your node modules node_modules/react-native-onyx/dist/Onyx.js.

POC
Screen.Recording.2024-11-16.at.14.55.36.mp4

@mananjadhav
Copy link
Collaborator

This one's tricky. Reviewing the issue as well as proposals. Will post an update by Monday.

@melvin-bot melvin-bot bot removed the Overdue label Nov 16, 2024
@mananjadhav
Copy link
Collaborator

Still reviewing.

Copy link

melvin-bot bot commented Nov 20, 2024

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

Copy link

melvin-bot bot commented Nov 21, 2024

@mananjadhav @CortneyOfstad this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

@melvin-bot melvin-bot bot added the Overdue label Nov 22, 2024
@FitseTLT
Copy link
Contributor

FitseTLT commented Nov 22, 2024

@luacmartins What do you think about my proposal here? I have proposed to auto updating and highlighting new chat messages in search the same as we are doing for expenses. I have also uploaded the result. It would also be nice if you self-assign as you are the head of search 👍 Thx

Copy link

melvin-bot bot commented Nov 25, 2024

@mananjadhav, @CortneyOfstad Huh... This is 4 days overdue. Who can take care of this?

@mananjadhav
Copy link
Collaborator

Reviewed this finally (took longer to navigate through all the code chaos). I think @FitseTLT's proposal should be enough to solve this one.

I don't think we should be updating onyx for this. But I'll let internal engineer confirm this one.

🎀 👀 🎀 C+ reviewed.

@melvin-bot melvin-bot bot removed the Overdue label Nov 26, 2024
@FitseTLT
Copy link
Contributor

FitseTLT commented Dec 5, 2024

It's ready now

Copy link

melvin-bot bot commented Dec 13, 2024

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

@CortneyOfstad
Copy link
Contributor

Hello! Just a heads up that I will be OOO starting this afternoon (December 20) and will be returning January 6th. A handful of folks on the BZ team will be online for a few days in between the 25th and the 1st, but we'll be operating with a skeleton crew. If any action is needed during that time, please post this issue in #expensify-open-source and someone on the team will jump in. Otherwise I will review when I am back on 1/6 — thanks!

@mananjadhav
Copy link
Collaborator

This has been deployed to production and ready for payout.

@mananjadhav
Copy link
Collaborator

BugZero Checklist:

  • [Contributor] Classify the bug:
Bug classification

Source of bug:

  • 1a. Result of the original design (eg. a case wasn't considered)
  • 1b. Mistake during implementation
  • 1c. Backend bug
  • 1z. Other:

Where bug was reported:

  • 2a. Reported on production (eg. bug slipped through the normal regression and PR testing process on staging)
  • 2b. Reported on staging (eg. found during regression or PR testing)
  • 2d. Reported on a PR
  • 2z. Other:

Who reported the bug:

  • 3a. Expensify user
  • 3b. Expensify employee
  • 3c. Contributor
  • 3d. QA
  • 3z. Other:
  • [Contributor] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake.

    Link to comment: NA

  • [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner.

    Link to discussion: NA

  • [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.

Regression Test Proposal Template

  • [BugZero Assignee] Create a GH issue for creating/updating the regression test once above steps have been agreed upon.

    Link to issue:

Regression Test Proposal

Precondition:

  • Have two accounts on the NewDot

Test:

  1. Open the App with an account A
  2. Click on "Search" on the bottom of the screen.
  3. Click on "Chats"
  4. From another account B with another device, send a message to the main account.
  5. New messages should be updated in real time (with highlighting) on chats section on main device.
  6. Verify the highlight on hover should be full length of the background.

Do we agree 👍 or 👎

@mananjadhav
Copy link
Collaborator

Quick bump @CortneyOfstad whenever you have time.

@CortneyOfstad
Copy link
Contributor

Sorry about that @mananjadhav! Getting this reviewed and sorted now!

@mananjadhav
Copy link
Collaborator

Thanks for helping @CortneyOfstad

@CortneyOfstad
Copy link
Contributor

CortneyOfstad commented Jan 7, 2025

Payment Summary

@FitseTLT — paid $250 via Upwork — requested refund of $125
@mananjadhav — to be paid $125 via NewDot

Regression Test

https://github.com/Expensify/Expensify/issues/458036

@github-project-automation github-project-automation bot moved this from CRITICAL to Done in [#whatsnext] #quality Jan 7, 2025
@CortneyOfstad CortneyOfstad changed the title [$250] Web - Search - New messages are not updated automatically in chats section [PAID 1/7/25] [$250] Web - Search - New messages are not updated automatically in chats section Jan 7, 2025
@mananjadhav
Copy link
Collaborator

@CortneyOfstad sorry I should’ve mentioned. There was a regression on this one. Hence the payout would be $125.

@CortneyOfstad CortneyOfstad reopened this Jan 7, 2025
@CortneyOfstad
Copy link
Contributor

Thanks for the heads up @mananjadhav! I've updated the payment summary and @FitseTLT — I've sent a refund request in Upwork. Please let me know once that is processed. Thank you and sorry for any hassle!

@FitseTLT
Copy link
Contributor

FitseTLT commented Jan 7, 2025

Thanks for the heads up @mananjadhav! I've updated the payment summary and @FitseTLT — I've sent a refund request in Upwork. Please let me know once that is processed. Thank you and sorry for any hassle!

BTW compared to the complexity of work on this issue the regression was a minor UI issue which I fixed immediately without any involvement of other contributors (without additional payments to solve it) but no problem if you insist on the refund please tell me the procedure to process it @CortneyOfstad 👍

@garrettmknight
Copy link
Contributor

$125 approved for @mananjadhav

@CortneyOfstad
Copy link
Contributor

Hi @FitseTLT — our process is that if there is a regression of any kind, the amount is adjusted by 50% reduction. Because of this, we hold everyone to the same standard, so we will need to move forward with the refund.

If you have any questions, or need further clarification — please let me know. Thank you!

@FitseTLT
Copy link
Contributor

Hi @FitseTLT — our process is that if there is a regression of any kind, the amount is adjusted by 50% reduction. Because of this, we hold everyone to the same standard, so we will need to move forward with the refund.

If you have any questions, or need further clarification — please let me know. Thank you!

Yep my question is how do I process the refund @CortneyOfstad Do you request it or is there a way to do it on my side?

@CortneyOfstad
Copy link
Contributor

Hi @FitseTLT — I requested it in Upwork. Are you able to see it on your side?

@FitseTLT
Copy link
Contributor

Hi @FitseTLT — I requested it in Upwork. Are you able to see it on your side?

Nope

@CortneyOfstad
Copy link
Contributor

Sorry @FitseTLT — I just sent it again via Upwork. Let me know if you're able to receive it this time. Thanks!

@FitseTLT
Copy link
Contributor

@CortneyOfstad I have refunded.

@mananjadhav
Copy link
Collaborator

@CortneyOfstad I think we're good to close this one out?

@CortneyOfstad
Copy link
Contributor

Confirmed the refund went through, so this can be closed! Thanks all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
Status: Done
Development

No branches or pull requests

8 participants