Fix Reload Button Crash #144

hreineck · 2021-07-28T20:08:51Z

Pressing the Reload button in rapid succession would crash
the loader by calling handleReloadClick multiple times
very quickly and getting to a state where the loader was running
and the isLoaded state variable was set incorrectly.
Adding this conditional should prevent handleReloadClick
from setting this at an unintended time.

Pressing the Reload button in rapid succession would crash the loader by calling `handleReloadClick` multiple times very quickly and getting to a state where the loader was running and the `isLoaded` state variable was set incorrectly. Adding this conditional should prevent `handleReloadClick` from setting this at an unintended time.

rgauss · 2021-07-28T20:22:40Z

src/components/OSCALLoader.js

@@ -56,8 +56,10 @@ export default function OSCALLoader(props) {
  };

  const handleReloadClick = () => {
-    setIsLoaded(false);
-    loadOscalData(oscalUrl);
+    if (isLoaded) {


Should we consider also visually disabling the reload button as part of this?

Good idea, I'll look into that.

Should be able to just set the disabled flag: https://material-ui.com/components/buttons/

… into EGRC-463

Added a state variable `contentLoaded` in OSCALLoader.js that tracks whether the content of the page has loaded or not. This state variable is passed into the renderer (Profile, Catalog, SSP, or Component), which sets it to true when the content has loaded. It is also passed into the OSCALLoaderForm component, which passes it into the button as the `disabled` prop, such that the button will be disabled initially, then enabled once the content has loaded.

In some circumstances, notably the profile tests, the prop `setContentLoaded` is undefined, causing failures. In order to resolve these failures and maintain the isolated functionality of the components, a conditional is added to check if the prop is defined before calling.

tuckerzp

Looks good to me so far. I had a few minor suggestions.

src/components/OSCALCatalog.js

src/components/OSCALComponentDefinition.js

src/components/OSCALSsp.js

Move the function setLoadedStates to Profile Utils and import it where it is used to reduce duplication of code. Also added brackets to if statements for readability.

Clean up the imports in profile utils to improve readability and address linting issues. Changed the import of fetchProfileModifications to no longer be default, because it's cleaner not to have a default when importing more than one function.

mikeisen1

Overall everything seems to work great, as the tests pass locally and I can see the added functionality. My only comment, besides my requested change, is that it seems we can still click the reload button multiple times, which increases the time it takes for the page to load. I'm not sure how (if you could) disable the reload button before a second click, but I just thought I'd mention this.

src/components/OSCALLoader.js

hreineck · 2021-08-03T20:09:44Z

Overall everything seems to work great, as the tests pass locally and I can see the added functionality. My only comment, besides my requested change, is that it seems we can still click the reload button multiple times, which increases the time it takes for the page to load. I'm not sure how (if you could) disable the reload button before a second click, but I just thought I'd mention this.

Agreed; it feels like it freezes on a mouse click and "buffers" the click even when the button is disabled. I'll look into this further, but has anyone worked with these types of React components and have any tips on making it run a bit smoother?

Change the `handleReloadClick` function to import and call the `setLoadedStates` util function, in order to reduce duplication. Also added a check for the `contentLoaded` state variable to the conditional in the function for readability and to cover certain edge cases.

Removing some left over debugging statements.

rgauss

Looking good overall. We could consider a slightly different approach that lets OSCALLoader handle state changes, passes that on to the OSCALLoaderForm, and passes an onResolutionComplete function to the OSCAL objects which are responsible for calling it when their resolution is done. I'll create a parallel draft PR to describe what I mean.

hreineck · 2021-08-04T21:13:22Z

Looking good overall. We could consider a slightly different approach that lets OSCALLoader handle state changes, passes that on to the OSCALLoaderForm, and passes an onResolutionComplete function to the OSCAL objects which are responsible for calling it when their resolution is done. I'll create a parallel draft PR to describe what I mean.

Thanks! This seems a bit cleaner. I'd like to see what others think, but I vote we go with your alternate approach.

…brary into EGRC-463 Merging from EGRC-463-alt to incorporate the alternate way of resolving the ticket: The state variable `isResourceLoaded` is now used to track when the viewer pages load, and a setter function is passed into each parent component. Resolved merge conflicts and a few readability/linter complaints.

mikeisen1 · 2021-08-05T20:17:34Z

I still see the issue, when on the profile viewer, of being able to click on the reload button multiple times (even when it's disabled). I initially wondered if this is an issue of simply displaying all the data on the screen. However, the background color of the reload button correctly reflects whether all the data has been rendered so that does not seem to be the issue. My guess is that we are incorrectly modifying isResolutionComplete, as it is somehow being modified before the profile rendering is complete. Other than that, everything else looks good and works as expected.

rgauss · 2021-08-06T16:40:33Z

I still see the issue, when on the profile viewer, of being able to click on the reload button multiple times (even when it's disabled). I initially wondered if this is an issue of simply displaying all the data on the screen. However, the background color of the reload button correctly reflects whether all the data has been rendered so that does not seem to be the issue. My guess is that we are incorrectly modifying isResolutionComplete, as it is somehow being modified before the profile rendering is complete. Other than that, everything else looks good and works as expected.

@mikeisen1, I've noticed that there are certain states where the browser is pretty much frozen, and I'm not sure the additional clicks are actually being registered, which doesn't seem to have to do with the reload functionality as much as resolving and rendering controls. Is that what you're seeing?

mikeisen1 · 2021-08-06T17:20:48Z

@mikeisen1, I've noticed that there are certain states where the browser is pretty much frozen, and I'm not sure the additional clicks are actually being registered, which doesn't seem to have to do with the reload functionality as much as resolving and rendering controls. Is that what you're seeing?

I took a look at it a little more and yeah it does seem that it is an issue of the browser being frozen. Is this something that should be resolved as part of a separate issue?

rgauss · 2021-08-06T17:55:12Z

@mikeisen1, I've noticed that there are certain states where the browser is pretty much frozen, and I'm not sure the additional clicks are actually being registered, which doesn't seem to have to do with the reload functionality as much as resolving and rendering controls. Is that what you're seeing?

I took a look at it a little more and yeah it does seem that it is an issue of the browser being frozen.

Ah, so only on the initial load? I think we can fix that by just setting the initial default of isResolutionComplete to false here.

rgauss · 2021-08-06T17:56:02Z

src/components/OSCALCatalog.js

@@ -17,6 +17,10 @@ const useStyles = makeStyles((theme) => ({
 export default function OSCALCatalog(props) {
  const classes = useStyles();

+  useEffect(() => {


Is useEffect needed, or can we just call props.onResolutionComplete()?

rgauss · 2021-08-06T17:56:45Z

src/components/OSCALLoader.js

@@ -29,6 +29,7 @@ const defaultOscalSspUrl =
 export default function OSCALLoader(props) {
  const [error, setError] = useState(null);
  const [isLoaded, setIsLoaded] = useState(false);
+  const [isResolutionComplete, setIsResolutionComplete] = useState(true);


The may need to change to false based on PR comments.

mikeisen1 · 2021-08-06T18:02:23Z

@mikeisen1, I've noticed that there are certain states where the browser is pretty much frozen, and I'm not sure the additional clicks are actually being registered, which doesn't seem to have to do with the reload functionality as much as resolving and rendering controls. Is that what you're seeing?

I took a look at it a little more and yeah it does seem that it is an issue of the browser being frozen.

Ah, so only on the initial load? I think we can fix that by just setting the initial default of isResolutionComplete to false here.

At least on my end, I notice that happens not only on the initial load, but also sometimes when I click on reload.

Changed the initialization of the state variable `isResolutionComplete` to initially be false, so that the button will appear disabled when the page is first loading in. Also made some minor cleanup to OSCALCatalog.js; removed the unnecessary `useEffect` call.

… into EGRC-463

hreineck self-assigned this Jul 28, 2021

hreineck linked an issue Jul 28, 2021 that may be closed by this pull request

Reload Button Crash on Profile Viewer #128

Closed

hreineck requested review from tuckerzp, rgauss, kylelaker, folksgl, mikeisen1, snorouzzadeh, zclarkEDC and Bronstrom July 28, 2021 20:09

rgauss suggested changes Jul 28, 2021

View reviewed changes

hreineck added 3 commits July 29, 2021 10:18

Merge branch 'develop' of github.com:EasyDynamics/oscal-react-library…

827cf9a

… into EGRC-463

hreineck force-pushed the EGRC-463 branch from b3824b2 to 56b1207 Compare July 30, 2021 14:31

hreineck requested a review from rgauss July 30, 2021 14:40

tuckerzp suggested changes Jul 30, 2021

View reviewed changes

src/components/OSCALCatalog.js Outdated Show resolved Hide resolved

src/components/OSCALComponentDefinition.js Outdated Show resolved Hide resolved

tuckerzp reviewed Jul 30, 2021

View reviewed changes

src/components/OSCALSsp.js Outdated Show resolved Hide resolved

hreineck added 2 commits August 2, 2021 11:55

Clean Up Util Function

1c643f6

Move the function setLoadedStates to Profile Utils and import it where it is used to reduce duplication of code. Also added brackets to if statements for readability.

Minor Cleanup

fe59c8b

Clean up the imports in profile utils to improve readability and address linting issues. Changed the import of fetchProfileModifications to no longer be default, because it's cleaner not to have a default when importing more than one function.

mikeisen1 suggested changes Aug 3, 2021

View reviewed changes

src/components/OSCALLoader.js Outdated Show resolved Hide resolved

hreineck force-pushed the EGRC-463 branch from 751ee2d to 5ca323a Compare August 3, 2021 21:25

hreineck and others added 2 commits August 3, 2021 17:30

Minor Cleanup

fc9a301

Removing some left over debugging statements.

Alternate approach to EGRC-463 / #128 / #144

2e52666

rgauss reviewed Aug 4, 2021

View reviewed changes

rgauss mentioned this pull request Aug 4, 2021

WIP: Alternate approach to EGRC-463 / #128 / #144 #148

Merged

hreineck requested review from rgauss and mikeisen1 and removed request for Bronstrom August 5, 2021 17:32

rgauss suggested changes Aug 6, 2021

View reviewed changes

hreineck added 2 commits August 9, 2021 10:30

Merge branch 'develop' of github.com:EasyDynamics/oscal-react-library…

4feeb45

… into EGRC-463

hreineck requested a review from rgauss August 9, 2021 14:46

rgauss approved these changes Aug 9, 2021

View reviewed changes

mikeisen1 approved these changes Aug 9, 2021

View reviewed changes

rgauss merged commit 1926853 into develop Aug 9, 2021

rgauss deleted the EGRC-463 branch August 9, 2021 21:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Reload Button Crash #144

Fix Reload Button Crash #144

hreineck commented Jul 28, 2021

rgauss Jul 28, 2021

hreineck Jul 28, 2021

rgauss Jul 28, 2021

tuckerzp left a comment

mikeisen1 left a comment

hreineck commented Aug 3, 2021

rgauss left a comment

hreineck commented Aug 4, 2021

mikeisen1 commented Aug 5, 2021

rgauss commented Aug 6, 2021

mikeisen1 commented Aug 6, 2021 •

edited

rgauss commented Aug 6, 2021

rgauss Aug 6, 2021

rgauss Aug 6, 2021

mikeisen1 commented Aug 6, 2021

Fix Reload Button Crash #144

Fix Reload Button Crash #144

Conversation

hreineck commented Jul 28, 2021

rgauss Jul 28, 2021

Choose a reason for hiding this comment

hreineck Jul 28, 2021

Choose a reason for hiding this comment

rgauss Jul 28, 2021

Choose a reason for hiding this comment

tuckerzp left a comment

Choose a reason for hiding this comment

mikeisen1 left a comment

Choose a reason for hiding this comment

hreineck commented Aug 3, 2021

rgauss left a comment

Choose a reason for hiding this comment

hreineck commented Aug 4, 2021

mikeisen1 commented Aug 5, 2021

rgauss commented Aug 6, 2021

mikeisen1 commented Aug 6, 2021 • edited

rgauss commented Aug 6, 2021

rgauss Aug 6, 2021

Choose a reason for hiding this comment

rgauss Aug 6, 2021

Choose a reason for hiding this comment

mikeisen1 commented Aug 6, 2021

mikeisen1 commented Aug 6, 2021 •

edited