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.

Sign up for GitHub

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

psp-7209 do not display acquisition file until all loading state is c… #3580

Merged
merged 1 commit into from
Nov 8, 2023
Merged

psp-7209 do not display acquisition file until all loading state is c… #3580

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion source/frontend/src/features/mapSideBar/acquisition/AcquisitionContainer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,12 @@ export const AcquisitionContainer: React.FunctionComponent<IAcquisitionContainer
};

// UI components
if (loadingAcquisitionFile || (loadingAcquisitionFileProperties && !isPropertySelector)) {
if (
loadingAcquisitionFile ||
(loadingAcquisitionFileProperties && !isPropertySelector) ||
file?.fileType !== FileTypes.Acquisition ||
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is odd. Do you know when this would be true?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, so essentially there is a race condition where when the file is loading the previous file is still in context. The state transition to set loading to false occurs before the new file is set into the context, and so this occurs.

Another solution would be to set the file in the context to null immediately during load.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This bug happens when you load a Research file into the sidebar (which populates the sidebar context), then without closing the sidebar go to an ACQ file (via the PIMS files tab of a property of the file). Then the container renders what is already in context thinking it is an ACQ file when it is not.

file?.id !== acquisitionFileId
) {
return <LoadingBackdrop show={true} parentScreen={true}></LoadingBackdrop>;
}

Expand Down
3 changes: 0 additions & 3 deletions source/frontend/src/features/mapSideBar/acquisition/AcquisitionView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,9 +76,6 @@ export const AcquisitionView: React.FunctionComponent<IAcquisitionViewProps> = (
const history = useHistory();
const match = useRouteMatch();
const { file, lastUpdatedBy } = useContext(SideBarContext);
if (!!file && file?.fileType !== FileTypes.Acquisition) {
throw Error('Context file is not an acquisition file');
}
Comment on lines -79 to -81
Copy link
Collaborator

Choose a reason for hiding this comment

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

I actually think this is a good error check. Without it we would have never found this bug/race condition. Just my opinion, but I would actually like to see this restored and a similar check added to ResearchView - but we can discuss on our Friday meeting

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we leave it in, it would be dead code, because the parent makes the same check. This if statement will never run with the recommended changes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

well, in theory this view could be embedded "anywhere" by another container but as I said would like us to discuss this use case for other views that make use of a context that might not have the information the view is expecting

const acquisitionFile: Api_AcquisitionFile = file as Api_AcquisitionFile;

// match for property menu routes - eg /property/1/ltsa
Expand Down
Loading