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

MCR-4120 Fix flickering from page remounting on RevisionSummary page #2441

Merged
merged 3 commits into from
May 13, 2024

Conversation

haworku
Copy link
Contributor

@haworku haworku commented May 10, 2024

Summary

This bug shows up when we have package submissions that have different contract names. This will happen when program names change.

Root cause: The bug appears because we render too early - first using data based on current revision, then as the client determines which package submission should actually be used for a previous submission, data updates. The infinite loop comes in because in cases where the contract name would also change between revisions (if programs had changed) this would trigger the useEffect in place that makes sure the proper page names are displaying in a higher level heading outside this component (usePage updates something in the header). This side effect would then cause page render going back to using old data, then it would correct itself, then the effect is triggered, and the cycle repeats in a loop.

Related issues

https://jiraent.cms.gov/browse/MCR-4120

Screenshots

video from buggy behavior is in bug ticket

Test cases covered

  • renders with correct submission name even when previous revisions have different programs - this is a failing test that will pass after the change. It makes sure the first render is the correct one and uses correct data starting out.
  • The fix for this bug also caused an existing test to fail. Would appreciate 👀 here.
    • renders the error indexed version 3 was the existing test that started failing after my change. Seems like (from looking at the test data) that test shouldn't have passed, we would expect version 3 to exist. There are three contract submissions on the test data. Made that change
    • Related, also wrote new test for Version 4 to 404 in order to still cover 404 case.

QA guidance

  • When a package has been resubmitted three times and each time the program name is changed, the previous submission for each version should load as expected

const name =
contract &&
contract?.packageSubmissions.length > Number(revisionVersion)
? contract.packageSubmissions.reverse()[revisionIndex]
Copy link
Contributor Author

@haworku haworku May 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This right here was also part of the problem - we were not using the same logic for the name and the data calculations - we should be filtering on CONTRACT_SUBMISSION also here like we do farther down in the file.

I changed things to calculate the target previous submission once at the top of the file-- and use that same variable for the name and any other data calculations for the page display.

customHeading: name,
})
Copy link
Contributor Author

@haworku haworku May 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw necessarily re-renders happening here, ideally we dont try to update (or clear) the heading in the <header/> at all until we are sure we have the proper submission name

@@ -77,35 +84,28 @@ export const SubmissionRevisionSummaryV2 = (): React.ReactElement => {
}

if (
!contract ||
contract.packageSubmissions.length <= Number(revisionVersion)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@haworku haworku marked this pull request as ready for review May 10, 2024 21:13
Copy link
Contributor

@JasonLin0991 JasonLin0991 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@@ -671,6 +671,13 @@ function mockContractPackageSubmittedWithRevisions(
}
}

// this package has val-like data - different versions have different descriptions and programs and package names
function mockContractPackageWithDifferentProgramsInRevisions (): Contract {
return{ "id": "e670adsfdfadsfc", "status": "RESUBMITTED", "createdAt": "2024-05-07T19:44:53.732Z", "updatedAt": "2024-05-07T19:44:53.732Z", "initiallySubmittedAt": "2024-05-07", "stateCode": "FL", "mccrsID": null, "state": { "code": "FL", "name": "Florida", "programs": [{ "id": "712277fb-f43f-4eb5-98c5-6c6a97509201", "name": "NEMT", "fullName": "Non-Emergency Medical Transportation", "__typename": "Program" }, { "id": "037af66b-81eb-4472-8b80-01edf17d12d9", "name": "PCCMe", "fullName": "Healthy Start MomCare Network, Inc.", "__typename": "Program" }, { "id": "5c10fe9f-bec9-416f-a20c-718b152ad633", "name": "MMA", "fullName": "Managed Medical Assistance Program", "__typename": "Program" }, { "id": "3b8d8fa1-1fa6-4504-9c5b-ef522877fe1e", "name": "LTC", "fullName": "Long-term Care Program", "__typename": "Program" }, { "id": "08d114c2-0c01-4a1a-b8ff-e2b79336672d", "name": "Dental", "fullName": "Prepaid Dental Health Program", "__typename": "Program" }], "__typename": "State" }, "stateNumber": 221, "__typename": "Contract", "draftRevision": null, "draftRates": null,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: It's a bit hard to read. We could probably format this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me merge this for demo - I'll circle back for this TY!

@haworku haworku merged commit a18274d into main May 13, 2024
28 checks passed
@haworku haworku deleted the mcr-4120-flicker branch May 13, 2024 14:37
@haworku haworku requested a review from JasonLin0991 May 13, 2024 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants