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

Chat - In offline, parent conversation is not highlighted. #54521

Open
2 of 8 tasks
IuliiaHerets opened this issue Dec 24, 2024 · 4 comments
Open
2 of 8 tasks

Chat - In offline, parent conversation is not highlighted. #54521

IuliiaHerets opened this issue Dec 24, 2024 · 4 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2

Comments

@IuliiaHerets
Copy link

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.78-1
Reproducible in staging?: Yes
Reproducible in production?: Yes
Issue reported by: Applause Internal Team
Device used: Redmi note 10s Android 13
App Component: Chat Report View

Action Performed:

  1. Go to https://staging.new.expensify.com/home
  2. Open a report
  3. Send a message and create a thread message
  4. Tap on header from link and navigate to parent message
  5. Note conversation is highlighted
  6. Go offline
  7. Repeat step 3 & step 4
  8. Note conversation is not highlighted

Expected Result:

In offline, parent conversation must be highlighted.

Actual Result:

In offline, parent conversation is not highlighted.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Bug6701658_1735019559394.Screenrecorder-2024-12-24-11-11-44-33.mp4

View all open jobs on GitHub

@IuliiaHerets IuliiaHerets added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Dec 24, 2024
Copy link

melvin-bot bot commented Dec 24, 2024

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

@daledah
Copy link
Contributor

daledah commented Dec 24, 2024

Edited by proposal-police: This proposal was edited at 2024-12-24 15:44:07 UTC.

Proposal

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

In offline, parent conversation is not highlighted.

What is the root cause of that problem?

  1. We're not allow navigating to parent within reportID

if (isVisibleAction && !isOffline) {
// Pop the chat report screen before navigating to the linked report action.
Navigation.goBack(ROUTES.REPORT_WITH_ID.getRoute(parentReportID, parentReportActionID), true);

  1. If we remove isOffline. In case users login to the App by deeplink, then go offline, the pages will be empty (has not been loaded yet)

const [reportActionPages] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS_PAGES}${reportIDWithDefault}`);

so the data is empty

if (pages.length === 0) {
return {data: id ? [] : sortedItems, hasNextPage: false, hasPreviousPage: false};
}

that shows the loading screen even though the data is loaded.

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

Solution 1: Disable navigating to parent if isOffline and pages is empty

We should update this condition in here and here

But this solution is not a best idea since even the page is not loaded, we still can show the parent message.

Solution 2:

Remove isOffline in 2 these places

Then update getContinuousChain logic to return sortedItems if page is empty

if (pages.length === 0) {
        const item = sortedItems.find((item) => getID(item) === id);
        return {data: id && !item ? [] : sortedItems, hasNextPage: false, hasPreviousPage: false};
    }

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

We should update getContinuousChain test to cover this case

What alternative solutions did you explore? (Optional)

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.

@FitseTLT
Copy link
Contributor

Proposal

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

Chat - In offline, parent conversation is not highlighted.

What is the root cause of that problem?

We are not linking to the parent report action id in offline mode here

if (isVisibleAction && !isOffline) {
// Pop the chat report screen before navigating to the linked report action.
Navigation.goBack(ROUTES.REPORT_WITH_ID.getRoute(parentReportID, parentReportActionID), true);

When comment linking to parent report action was implemented a regression occurred where if you create threads offline and try to comment link back to parent actions it was showing skeleton loading although the report actions where available in onyx so we disabled comment linking to parent actions in here by adding conditions in all three places here here and here
But the reason it was showing loading indicator was because usePaginatedReportActions was returning empty data (reportActions list) because when linking to report actions we return empty if page is empty here
if (pages.length === 0) {
return {data: id ? [] : sortedItems, hasNextPage: false, hasPreviousPage: false};

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

Remove the isOffline checks in all three places here here and here

To avoid the skeleton loading problem we should only return empty if the linked report action doesn't exist in sortedItems list

if (pages.length === 0) {
return {data: id ? [] : sortedItems, hasNextPage: false, hasPreviousPage: false};

 if (pages.length === 0) {
        return {data: id && !sortedItems.some((action) => getID(action) === id) ? [] : sortedItems, hasNextPage: false, hasPreviousPage: false};
    }

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

We can create a test for getContinuousChain by passing empty pages and proper id param to assert that it doesn't return empty if the action exists in sortedItems

What alternative solutions did you explore? (Optional)

@daledah
Copy link
Contributor

daledah commented Dec 25, 2024

@FitseTLT Your new proposal is same as mine 🙏. You deleted the previous one after reading my proposal.

You just added another place that can be easy to figure out (by searching if (isVisibleAction && !isOffline) in our code). I think we can create the util function to prevent duplicated code.

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
Projects
None yet
Development

No branches or pull requests

4 participants