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

[$250] Chat - "New Messages" badge is displayed when navigating to the link of a message #52782

Closed
3 of 8 tasks
IuliiaHerets opened this issue Nov 19, 2024 · 18 comments
Closed
3 of 8 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors

Comments

@IuliiaHerets
Copy link

IuliiaHerets commented Nov 19, 2024

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Version Number: 9.0.64-0
Reproducible in staging?: Y
Reproducible in production?: Y
Issue reported by: Applause Internal Team

Action Performed:

  1. Open the Expensify app.
  2. Open any chat with long message history.
  3. Scroll up until the bottom of the chat stop being visible and long tap on a message.
  4. Select "Copy Link"
  5. Navigate to a different chat and paste the message´s link.
  6. Tap on the just sent link.
  7. Verify you are redirected to the correct chat and message and that the "New Messages" badge is not displayed.

Expected Result:

When navigating to a message´s link, the user should land on the correct chat, the message should be highlighted and the "New Messages" badge should not be displayed.

Actual Result:

When the user navigates to the link of a message on a chat with long history, the "New Messages" badge can be seen for some time when landing on the message.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Bug6669706_1732032298305.Badge.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021859357029005874727
  • Upwork Job ID: 1859357029005874727
  • Last Price Increase: 2024-12-04
Issue OwnerCurrent Issue Owner: @thesahindia
@IuliiaHerets IuliiaHerets added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Nov 19, 2024
Copy link

melvin-bot bot commented Nov 19, 2024

Triggered auto assignment to @greg-schroeder (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.

@greg-schroeder greg-schroeder moved this to Bugs and Follow Up Issues in [#whatsnext] #expense Nov 20, 2024
@greg-schroeder greg-schroeder added the External Added to denote the issue can be worked on by a contributor label Nov 20, 2024
@melvin-bot melvin-bot bot changed the title Chat - "New Messages" badge is displayed when navigating to the link of a message [$250] Chat - "New Messages" badge is displayed when navigating to the link of a message Nov 20, 2024
Copy link

melvin-bot bot commented Nov 20, 2024

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

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

melvin-bot bot commented Nov 20, 2024

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

@hunghinh2000
Copy link

Proposal

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

When a user follows a link to a specific message in a chat with long history, the "New messages" badge incorrectly appears temporarily while landing on the target message, even though it shouldn't be shown in this context since the user is intentionally viewing an older message.

What is the root cause of that problem?

The root cause appears to be in the ReportActionsList component where the floating message counter visibility logic doesn't properly handle the case of comment linking. The current implementation checks the scroll position against a threshold to determine whether to show the badge, but doesn't take into account that the user arrived via a direct message link.
Specifically in:

/**
* Show/hide the new floating message counter when user is scrolling back/forth in the history of messages.
*/
const handleUnreadFloatingButton = () => {
if (scrollingVerticalOffset.current > VERTICAL_OFFSET_THRESHOLD && !isFloatingMessageCounterVisible && !!unreadMarkerReportActionID) {
setIsFloatingMessageCounterVisible(true);
}
if (scrollingVerticalOffset.current < VERTICAL_OFFSET_THRESHOLD && isFloatingMessageCounterVisible) {
if (readActionSkipped.current) {
readActionSkipped.current = false;
Report.readNewestAction(report.reportID);
}
setIsFloatingMessageCounterVisible(false);
}
};

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

We should make the following specific changes to the ReportActionsList component:

  1. Add a new ref to track whether we're in a linked message view:
// Converts the ID to a boolean - true if we have a linked message ID, false if not
// This ref will track whether we're currently in the initial view of a linked message
const isInitialLinkedMessageView = useRef(!!linkedReportActionID);
  1. Modify the handleUnreadFloatingButton function to consider this state:
const handleUnreadFloatingButton = () => {
    // Don't show the floating button if we're viewing a linked message and haven't scrolled away yet
    if (isInitialLinkedMessageView.current) {
        return;
    }

    if (scrollingVerticalOffset.current > VERTICAL_OFFSET_THRESHOLD && !isFloatingMessageCounterVisible && !!unreadMarkerReportActionID) {
        setIsFloatingMessageCounterVisible(true);
    }

    if (scrollingVerticalOffset.current < VERTICAL_OFFSET_THRESHOLD && isFloatingMessageCounterVisible) {
        if (readActionSkipped.current) {
            readActionSkipped.current = false;
            Report.readNewestAction(report.reportID);
        }
        setIsFloatingMessageCounterVisible(false);
    }
};
  1. Add logic to detect when user has scrolled away from the linked message:
const trackVerticalScrolling = (event: NativeSyntheticEvent<NativeScrollEvent>) => {
    scrollingVerticalOffset.current = event.nativeEvent.contentOffset.y;
    
    // If we detect significant scroll movement, we're no longer in the initial linked message view
    if (isInitialLinkedMessageView.current && Math.abs(event.nativeEvent.contentOffset.y) > VERTICAL_OFFSET_THRESHOLD) {
        // User has scrolled away, so we're no longer in linked message view
        isInitialLinkedMessageView.current = false;
    }
    
    handleUnreadFloatingButton();
    onScroll?.(event);
};
  1. Reset the linked message state when the report changes:
useEffect(() => {
    // Reset linked message state when report changes
    isInitialLinkedMessageView.current = !!linkedReportActionID;
}, [report.reportID]);

These changes will:

  • Prevent the "New messages" badge from appearing when initially viewing a linked message.
  • Allow the badge to appear normally once the user starts scrolling through the chat.
  • Reset the behavior appropriately when switching between different chats.
  • Maintain all existing functionality for normal chat navigation.

The solution uses a ref instead of state to avoid unnecessary re-renders and keeps track of the initial linked message context until the user actively scrolls away from it. This approach provides a smooth user experience while being efficient in terms of performance.

What alternative solutions did you explore? (Optional)

  1. Considered modifying the scroll threshold logic, but this wouldn't properly handle the specific use case of comment linking
  2. Considered tracking the initial render vs subsequent scrolls, but this would be more complex to maintain and could introduce other edge cases

@me-ZaidAli
Copy link

Proposal

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

new message floater showing when we link to a chat message

What is the root cause of that problem?

The problem is in these lines in canScrollToNewerComments.

// When performing comment linking, initially 25 items are added to the list. Subsequent fetches add 15 items from the cache or 50 items from the server.
// This is to ensure that the user is able to see the 'scroll to newer comments' button when they do comment linking and have not reached the end of the list yet.
const canScrollToNewerComments = !isLoadingInitialReportActions && !hasNewestReportAction && sortedReportActions.length > 25 && !isLastPendingActionIsDelete;
return (
<>
<FloatingMessageCounter
isActive={(isFloatingMessageCounterVisible && !!unreadMarkerReportActionID) || canScrollToNewerComments}
onClick={scrollToBottomAndMarkReportAsRead}

When we link to a comment or a chat message in general, isFloatingMessageCounterVisible stays false since it's value depends on unreadMarkerReportActionID which is only set when we get a new message and it isn't in view threshold (has got nothing to do with message or comment linking). canScrollToNewerComments becomes true in case of message linking since it's being used to show New messages floater in case of comment linking. So when we linked to a specific message in a chat, canScrollToNewerComments becomes true and starts showing "New Message" floater.
In the video the floater disappears after showing up for a split second. It's because canScrollToNewerComments has !hasNewestReportAction as one of the conditions for it to be true and if we look at hasNewestReportAction
const hasNewestReportAction = lastAction?.created === lastVisibleActionCreated;

it has got a condition to check whether last action's timestamp matches report's last visible action's timestamp and since data is being loaded in a paginated way, the condition get's set to false (which sets !hasNewestReportAction to true) since the last action for the chat isn't loaded yet and once it's loaded, condition gets set to true (which sets !hasNewestReportAction to false) and so floater shows up for a split second.

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

Here I am assuming that we need to show New Message floater for comment linking. In that case we can distinguish threads from normal chats and make sure canScrollToNewerComments is only evaluated to true when we are linking to a comment. To achieve that we can create a isThread

    const isThread = parentReportActionForTransactionThread ?? parentReportAction;

here parentReportActionForTransactionThread is only defined when we are in a transaction thread and parentReportAction is defined in both transaction threads and chat threads so by using this condition we can distinguish whether we are being linked to a thread or a normal chat. Now using this in canScrollToNewerComments

const canScrollToNewerComments = isThread && !isLoadingInitialReportActions && !hasNewestReportAction && sortedReportActions.length > 25 && !isLastPendingActionIsDelete;

This'll stop setting canScrollToNewerComments to true in normal chat linking and will resolve the issue.

What alternative solutions did you explore? (Optional)

Alternate would be to not show New message floater in any type of linkings. In that case we'll remove canScrollToNewerComments altogether to resolve the issue.

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

Copy link

melvin-bot bot commented Nov 26, 2024

@greg-schroeder, @thesahindia Huh... This is 4 days overdue. Who can take care of this?

@melvin-bot melvin-bot bot added the Overdue label Nov 26, 2024
@greg-schroeder
Copy link
Contributor

greg-schroeder commented Nov 26, 2024

Proposal review underway cc @thesahindia

@thesahindia
Copy link
Member

These changes will:

  • Prevent the "New messages" badge from appearing when initially viewing a linked message.
  • Allow the badge to appear normally once the user starts scrolling through the chat.
  • Reset the behavior appropriately when switching between different chats.
  • Maintain all existing functionality for normal chat navigation.

@hunghinh2000, what will happen if I am in the linked message view and receive a new message? Will I get the 'New messages' badge?

@melvin-bot melvin-bot bot removed the Overdue label Nov 26, 2024
@thesahindia
Copy link
Member

thesahindia commented Nov 26, 2024

Here I am assuming that we need to show New Message floater for comment linking.

We don't want it to appear when navigating to the linked message.

Alternate would be to not show New message floater in any type of linkings. In that case we'll remove canScrollToNewerComments altogether to resolve the issue.

@me-ZaidAli, could you please elaborate that? Are you implying that we don't need canScrollToNewerComments? If yes, could you please explain why it was added and why we don't need it?

Copy link

melvin-bot bot commented Nov 27, 2024

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

@me-ZaidAli
Copy link

@thesahindia the canScrollToNewerComments is basically set to true in case of linking to a thread comment solely and the over it says that as well. My main solution is to segregate thread linking from message linking and the alternate is to remove canScrollToNewerComments if we don't want it in any type of linking.

// When performing comment linking, initially 25 items are added to the list. Subsequent fetches add 15 items from the cache or 50 items from the server.
// This is to ensure that the user is able to see the 'scroll to newer comments' button when they do comment linking and have not reached the end of the list yet.
const canScrollToNewerComments = !isLoadingInitialReportActions && !hasNewestReportAction && sortedReportActions.length > 25 && !isLastPendingActionIsDelete;

@thesahindia
Copy link
Member

@greg-schroeder, could we please confirm the expected result here? It seems like the design was to show the "New messages" badge after the redirection.

Found these steps from the PR that implemented this feature:

  1. Continue using the chat from Test 1.
  2. From Account A, click on the first link. Verify redirection to the exact message.
  3. After redirection, a popup should appear at the top of the screen (similar to new message popup).
  4. Click on this popup.
  5. Verify navigation to the end of the list (newest messages).

Copy link

melvin-bot bot commented Dec 3, 2024

@greg-schroeder @thesahindia 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 Dec 3, 2024
Copy link

melvin-bot bot commented Dec 3, 2024

@greg-schroeder, @thesahindia Eep! 4 days overdue now. Issues have feelings too...

@greg-schroeder
Copy link
Contributor

Asking the team @thesahindia

Copy link

melvin-bot bot commented Dec 4, 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 Dec 5, 2024

@greg-schroeder, @thesahindia 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

@greg-schroeder
Copy link
Contributor

Team agrees expected behavior per #52782 (comment)

@melvin-bot melvin-bot bot removed the Overdue label Dec 6, 2024
@github-project-automation github-project-automation bot moved this from Bugs and Follow Up Issues to Done in [#whatsnext] #expense Dec 6, 2024
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. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors
Projects
Status: Done
Development

No branches or pull requests

5 participants